Feature awilix and missive.js #3

Plopix · 2024-11-15T08:13:05Z

No description provided.

andrelandgraf · 2024-11-15T22:37:20Z

.vscode/settings.json

@@ -20,6 +20,7 @@
    "**/dist": true,
    "**/node_modules": true,
    "**/build": true,
-    "**/.cache": true
+    "**/.cache": true,
+    "website/data": true


I actually like to see all files in VS Code. It's too easy to forget that files are hidden and then get confused. For instance, I inspect node_modules rather often. How would you debug the cached content and DB in website/data when the folder is excluded by default? 🤔

no problem, I go to the settings when I want to do that as it's probably only 20% of the time... but yes let's not hide it by default.

Sweet, let's remove it before merging!

andrelandgraf · 2024-11-15T22:40:32Z

website/app/core/configs.server.ts

+import type { Schema, ZodTypeDef } from 'zod';
+import { z } from 'zod';
+
+export const validateConfigOrExit = <T, I>(schema: Schema<T, ZodTypeDef, I>, intent: I): T => {


I really like this, even just for the existing env.server.ts file. It's way better than my current manual parsing of .env.

andrelandgraf · 2024-11-15T22:42:22Z

website/app/core/container.server.ts

+    cache: 'all',
+    shortCircuit: true,
+  });
+  container.cradle.queryBus.register('FetchSpeakersWithTalks', container.cradle.fetchSpeakersWithTalksHandler);


This one is weird to me. Must all queries across the app be registered here?

yes we need to setup the Bus

Why doesn't every handler have its own bus, wrapping the handler export in its own domain/ or modules/ file?

You mentioned one of the motivations for this is that it "scales" once the codebase becomes bigger, but having one bus that needs to implement all requirements of all fetch requests and needs to be the same for all handlers doesn't look like it would scale. It sounds more like Redux or Axios middleware, which historically usually ends up in a big mess with folks adding chaotic code to them.

For instance, how would you solve: 'this handler data should never be cached' or 'this handler, I want to log more than just the default' use cases in the current implementation?

The goal of a bus is having a centralized piece of code where you can add middlewares (common to all handlers).

It is a bit like having multiple express app for every routes ;)

of course, here you can have many subfolders in domain/ to organize with many engineers. same in contracts/ (here the project is small).

It does scale, there is no doubt about that.

The thing is that middlewares are nice but they are not the solution for everything. Nothing forbids your to repeat some logics in the handler like your would do without a bus. At the end the handler is the simple function that exists right now. Codebase can become a gigantic mess with modules everywhere too.

it's a pattern here, it does not solve everything: https://martinfowler.com/bliki/CQRS.html

andrelandgraf · 2024-11-15T22:44:54Z

website/app/core/container.server.ts

+      signingKey: process.env.INNGEST_SIGNING_KEY,
+      eventKey: process.env.INNGEST_EVENT_KEY,
+    },
+    sessionSecret: process.env.SESSION_SECRET || '',


Instead of MainConfig, why don't we define const config as Partial<MainConfig>. We get the same autocompletion but don't have to do the || '' and || 'xxx' to avoid type errors.

it's WIP here, we should not have so many || '' and || 'xxx' should be optional when it's optional or required but most likely not default value unless it makes sense.

Can we fix this before merging? Remove the || '' and || 'xxx' everywhere and make sure that optional configuration options properly disable the services (like resend) when not set instead of throwing.

andrelandgraf · 2024-11-15T22:45:37Z

website/app/core/container.server.ts

+      apiKey: process.env.RESEND_API_KEY,
+    },
+    luma: {
+      apiKey: process.env.LUMA_API_KEY || 'xxx',


Instead of 'xxx', I would much rather define these entities as optional in the zod schema.

andrelandgraf · 2024-11-15T22:58:11Z

website/app/infrastructure/create-logger.server.ts

@@ -0,0 +1,65 @@
+import Signale from 'signale';


What's /infrastructure in your mind? Why is logger not a "domain"?

to me createLogger is infrastructure as it relies on a specific implementation. LoggerInterface (contracts) is the definition of a Logger.

we could have signale, pino, or console logger (all infra). they respect the interface.

agreeing with @Plopix, as a logger deals with I/O it should be part of infrastructure

andrelandgraf · 2024-11-15T22:58:49Z

website/app/infrastructure/create-mailer.server.ts

@@ -0,0 +1,32 @@
+import { Resend } from 'resend';


Same here, are you trying to abstract the SaaS / third-party away from the domain?

locally we use NodeMailer right, we could instanciate nodemailer in development while resend in prod. both respecting the contract. In test we could have a mailer that write in a file for instance.
Interchangeable implementation.

I see the benefit of that. That's pretty clever. But I don't think you need a fancy folder name for that haha

modules/mail is the thing used by the app, which can be instantiated via modules/node-mailer or modules/resend? Just so I understand, why would that not work?

I see the benefit of that. That's pretty clever. But I don't think you need a fancy folder name for that haha

It is part of some architecture principle named hexagonal architecture ;) it helps to decouple the code and understanding where are the things :)

yes it's decoupling the implementation and the interfaces to me. the domain should never know about "resend", "pocketbase" or any implementation, it should rely on interfaces (contracts) only.

andrelandgraf · 2024-11-15T22:59:33Z

website/app/infrastructure/create-server-timings-profiler.server.ts

@@ -3,48 +3,19 @@
 * https://gist.github.com/jacobparis/1e428524be3a31096ba3ecb35a7a15bb


Mh, so modules/ still exists. I thought domain/ replaces modules? What's the difference?

migration is not completely done, I would keep modules for frontend /shareable stuff maybe. I usually don't have modules.

andrelandgraf · 2024-11-15T23:00:20Z

website/app/infrastructure/session/create-session-manager.server.ts

@@ -0,0 +1,79 @@
+import { createCookieSessionStorage } from '@remix-run/node';


Why is session management infrastructure? 🤔

again the create is infrastructure, the contract is not.
it relies on Remix, therefore it's infra to me

andrelandgraf · 2024-11-15T23:04:03Z

website/app/routes/2024-10-05-hackathon-at-sentry.tsx


 export { headers } from '~/modules/header.server';

 export { meta };

-export function loader() {
-  return eventDetailsLoader('2024-10-05-hackathon-at-sentry');
+export async function loader({ context }: LoaderFunctionArgs) {


Okay, this is obviously where all the legwork pays off. It's more lines but very nice to read and I can see how this would pay off in the action and loader functions when they get more complex. But I am also a bit more afraid of debugging this in case something goes wrong because it's not purely a plain function that is being called. It's a bunch of layers configured elsewhere, influenced by the global context.

well that would/should exclusively be the handler which is a plain function.
yes you have the middleware too, but most likely the handler.

here one can argue that debugging is more complex cause instances of anything can come from anywhere via import.

Exactly, and that's precisely the point where I am like, this is probably overkill...

This & my concern that middlewares now can't really be handler specific, right?

From previous comment: "For instance, how would you solve: 'this handler data should never be cached' or 'this handler, I want to log more than just the default' use cases in the current implementation?"

If things like caching aren't middleware but plain function calls that return stuff, then you can provide the required arguments in each loader & action function to alter the caching time or logging logic. Each loader and action function contains all the logic, all the function calls, and can be managed independently.

First here loader/action are framework-specific, so importing remix-specific stuff makes it "not decoupled" enough.
Loader/action are controllers, the glue between the framework and the domain/infra.

And I think it's a wrong approach to think about it this way. Per definition, a middleware is not "handler specific" there is no point having that. if you try to do that, there is a flaw somewhere.

Also, you don't need a middleware all the time, you can still manage anything you want in the handler and have logic in the handlers. (like you do today at the end)

Also, you can disable the cache per query/command for instance (already possible in missive.js)

If you need different cache storage per query/command (assuming it would make sense...based on a different discussion we 've got) you could have 2 cacher middlewares 1 for redis and 1 for in memory, both using the built-in middleware but with 2 different adapters and they would have each "per intent" configuration.

also I could see a world where you wrap the dispatch in cachified for instance, if you prefer. A bit weird but not invalid.

Missive.js is not a dropped-in replacement for everything it's a lib that helps implement the paradigm with a bus CQRS. if you have 246 queries/ commands, I am pretty confident it would bring more value that it would remove.

In short the goal, on any projects, the way I envisioned them, is:

to have contracts/interfaces you share across the app disconnected from any implementations

to have domain(s) folders/files that rely ONLY on the contracts/interfaces

to have infrastructure(s) folders/files that are the actual implementations (respecting the interfaces)

That is before any Bus, CQRS or, anything.
Then Missive.js brings CQRS, a paradigm on top that works well with the above approach.

The modules approach works well, it's not bad, but it does not scale the same way it's actually bloated when you think about it, not easily testable, not easily interchangeable, not yelling what it does but more what tool it is, etc.

It's just another approach, I do think hexagonal architecture is better suited for big professional projects, and yes overkill for small projects like this one.

my 2 cents ;) I have implemented hundreds of projects some with CQRS some without it. Like any tech, we need to use it when it makes sense ;-)

=> here, this PR is on a small project and was to show/share another way.

andrelandgraf · 2024-11-17T19:17:54Z

Happy to try out the service bus implementation with missive & dependency injection with awilix. I'm curious to see how both feel when maintaining it over time. However, I really want to keep my general folder setup the same. This PR also changes the files & folder convention that I set up and I don't see a reason to change that. Let's keep it as "everything is a module".

Blockers before merging:

remove contracts, domains, infrastructure folders and make all new features modules/.
remove "xxx" and "" default values for unset optional environment variables.
make the environment parsing a utils function that can also be called outside of the DI world for the places where Remix doesn't provide us with a way for DI.

Plopix · 2024-11-18T02:14:13Z

That was a nice and interesting discussion, let's do it step by step:

DIC with Awilix
Config with Zod

#4

Closing this one

Next is Missive.js

And then maybe, a bit of change in the modules folder but your project, your home, your decision ;-)

Plopix added 3 commits November 15, 2024 00:09

feat: bring proper dependency injection with Awilix

0038510

feat: mailer, inngest, using the dic

857d778

chore: dic almost everywhere

3be01b0

andrelandgraf reviewed Nov 15, 2024

View reviewed changes

Plopix mentioned this pull request Nov 18, 2024

feat: bring dependency injection container and config validation via zod #4

Merged

3 tasks

Plopix closed this Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature awilix and missive.js #3

Feature awilix and missive.js #3

Plopix commented Nov 15, 2024

andrelandgraf Nov 15, 2024

Plopix Nov 16, 2024

andrelandgraf Nov 17, 2024

andrelandgraf Nov 15, 2024

andrelandgraf Nov 15, 2024

Plopix Nov 16, 2024

andrelandgraf Nov 16, 2024

anaelChardan Nov 16, 2024

Plopix Nov 16, 2024

andrelandgraf Nov 15, 2024

Plopix Nov 16, 2024

andrelandgraf Nov 17, 2024

andrelandgraf Nov 15, 2024

Plopix Nov 16, 2024

andrelandgraf Nov 15, 2024

Plopix Nov 16, 2024

anaelChardan Nov 16, 2024

andrelandgraf Nov 15, 2024

Plopix Nov 16, 2024

andrelandgraf Nov 16, 2024

anaelChardan Nov 16, 2024

Plopix Nov 16, 2024

andrelandgraf Nov 15, 2024

Plopix Nov 16, 2024

andrelandgraf Nov 15, 2024

Plopix Nov 16, 2024

andrelandgraf Nov 15, 2024

Plopix Nov 16, 2024

andrelandgraf Nov 16, 2024

andrelandgraf Nov 16, 2024

Plopix Nov 16, 2024

andrelandgraf commented Nov 17, 2024 •

edited by Plopix

Loading

Plopix commented Nov 18, 2024

		@@ -3,48 +3,19 @@
		* https://gist.github.com/jacobparis/1e428524be3a31096ba3ecb35a7a15bb

		@@ -0,0 +1,79 @@
		import { createCookieSessionStorage } from '@remix-run/node';

Feature awilix and missive.js #3

Feature awilix and missive.js #3

Conversation

Plopix commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrelandgraf commented Nov 17, 2024 • edited by Plopix Loading

Plopix commented Nov 18, 2024

andrelandgraf commented Nov 17, 2024 •

edited by Plopix

Loading