Introduce directories.srcDir for configuring source code location

thetutlage commented 1 week ago

Okay, so here is a bit of back story to explain the change I am trying to make.

The goal

The goal is to eventually use a JIT (TS-Node + SWC) to run the backend API in development. Once this change goes live, there won't be any dist folder in the project. We should discover resources from the src folder.

Now, this change must be done inside the loaders so that instead of importing files from dist, loaders will import files from the src folder. BUT ONLY IF IT IS AN APP (NOT A PACKAGE).

Avoiding breaking everything

This is going to be a two-fold change. First in the medusa/medusa package and next in the starter kit (we will configure TS-Node + SWC in starter kit). So, in the meantime, old starter kits with new medusa package will break.

So what I am doing is, introducing a temporary config called directories.srcDir. The default value of this config is dist. So old apps will continue to discover the resources from the dist folder.

And for the new apps (using JIT compiler), we will define directories.srcDir under the medusa-config.js file.

Maybe in couple of days when everything is stable. We can remove this config altogether.

changeset-bot[bot] commented 1 week ago

⚠️ No Changeset found

Latest commit: 0296c0968acfd46c41941ac1b897a17961812d35

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel[bot] commented 1 week ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
medusa-dashboard	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 21, 2024 9:14am

3 Ignored Deployments

thetutlage commented 1 week ago

Alright. After an intense session with @sradevski this PR is going to get bigger, infact a log bigger 😇

thetutlage commented 1 week ago

Okay. So here's the GIST of the changes.

In a huddle, me and @sradevski discovered that the resources from the starter kit are exposed as a plugin to the rest of the codebase. Here's the block of code that returns starter kit resources as a plugin. https://github.com/medusajs/medusa/blob/becf903cea42ba4b69d9aa9b3fdbd206dc6c765f/packages/medusa/src/loaders/helpers/resolve-plugins.ts#L115-L134

So our goal was to make all the *loader functions imports their specific resources from the list of plugins loaded via the resolvePlugins methods.

Also, at the same time we found some inconsistencies in the codebase. For example:

In case places, we were using the AwilixContainer as a type and in other places we were using MedusaContainer. I have clean up some reference, but not all. Can do that in a separate PR.
Some parts of our codebase accepts ConfigModule, some accepts a subset of ConfigModule. Our goal is to be as specific as possible with what a function accepts and pass exactly that info at the time of calling the function. This makes it a little easier to reason about the code. Again, haven't cleaned up everything.

Finally, we are now able to load and register routes from the starter kit after the last commit.

thetutlage commented 1 week ago

Working on fixing the unit tests

thetutlage commented 1 week ago

Tests were passing before merging develop and right now it seems like some payments related tests are failing. Can someone point me in the direction on how to fix it?

olivermrbl commented 1 week ago

Nice clean-up!

Regarding the added back Subscribers and API Routes registration, would you be up for running a few tests in your own project?

You can publish snapshot versions of the changes in your PR by commenting /snapshot-this. Then, after ~2-3 mins a snapshot version will be added to the PR as a comment.

Not super important though, but thought I'd share so you know about the snapshots going forward.

adrien2p commented 1 week ago

One question, couldn't we use something like process[Symbol.for("ts-node.register.instance")] in order to identify if we are running under ts-node or not? In that case, when true we know that we have to discover the resources from the src and otherwise from the dist, we can still store it in the config for our convinience but don't think it should be made available in the medusa config wdyt?

thetutlage commented 1 week ago

@adrien2p

Yeah we can do that. But, if you ask me, I would love the internals of the Medusa to be not such aware of where the source code lives. It should come from some config.

The end-user don't have to write this config (if we want to keep things minimal), but the core logic should be free of knowledge about the directory structure.

I know we are not there yet, but atleast that's my dream end state.

adrien2p commented 1 week ago

@adrien2p

Yeah we can do that. But, if you ask me, I would love the internals of the Medusa to be not such aware of where the source code lives. It should come from some config.

The end-user don't have to write this config (if we want to keep things minimal), but the core logic should be free of knowledge about the directory structure.

I know we are not there yet, but atleast that's my dream end state.

What I thought is that it could be part of the config loader, not getting this value from the user but instead conpute it, in that way, the core is not aware of it and can still be based on the config module but we do not make it available to the end user. does that make sense?

adrien2p commented 1 week ago

We need to think about adding some tests as well as mentioned by @riqwan just to prevent regression, for the router there is some tests already i think

thetutlage commented 1 week ago

Most of the feedback has been implemented in this PR. There are a couple of things that I want to understand from others on a call.

So I will merge this PR, do the call and will create a new PR (if needed). Otherwise, this branch keeps on getting stale and I have resolve merge conflicts

carlos-r-l-rodrigues commented 1 week ago

great stuff 🥇

medusajs / medusa