fix!: Convert project to output ESM

[gregersrygg]

Description

The latest release (0.3.0), and possibly the release before (0.2.6) broke TypeScript compatibility.

A few months ago the project.json was changed to "type": "module". This changes how tools interpret the build output of the project, but the built output is still CommonJS.

We discovered this when trying to update the dependency though renovate bot, but it failed to update because TypeScript couldn't find the exported types. From the renovate bot statistics, it looks like everyone else is having the same problem. Here is the adoption rate of 0.3.0: .

I think it's a good thing to switch to ESM as most of the industry already has, so rather than revert the change I attempted to complete the switch to output ESM javascript and type definitions. This forces consumers of the project to switch their project to use module in their project as well, so this is a breaking change. Ideally the "type": "module" change should be reverted for 0.3.0 and 0.2.6 to avoid fix the breaking change there.

The changes I made are from the recommendations from Sindre Sørhus on how to convert a TypeScript module: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

[github-actions]

👋 @gregersrygg

Welcome to the Microcks community! 💖

Thanks and congrats 🎉 for opening your first pull request here! Be sure to follow the pull request template or please update it accordingly.

Hope you have a great time there!

🌟 ~~~~~~~~~ 🌟

📢 If you like Microcks, please ⭐ star ⭐ our repo to support it!

🙏 It really helps the project to gain momentum and credibility. It's a small contribution back to the project with a big impact.

[lbroudoux]

Hi @gregersrygg

Thanks for raising this one. You're right: I had to make that change to make Keycloak tests possible using the keycloak-testcontainer lib.

However, I didn't see the regression as the microcks-testcontainers-node-nest-demois still working with themicrocks-testcontainers` used in Typescript (as see here for example). Does it mean that we're doing things the wrong way in this demo repository?

I'm also a 💯 partisan of making this lib available to Typescript users! I'll go through your changes and review and will probably ask some dumb questions as I think you have more knowledge that I have on this topic.

[lbroudoux]

I've been through the proposed changes and my concern is that we tend to drift from the way the official testcontainers modules are configured and written.

Wouldn't it be worth going though the original reason I made this change (see slemke/keycloak-testcontainer#149) and check if it's still needed? Also, it looks like testcontainers is also moving from Jest to Vitest and maybe that would solve our original issue and allow to rollback to the settings we had on 0.2.5?...

[lbroudoux]

So we have to reference the *.js files now?

[gregersrygg]

Yes, even though it's typescript:

You must use a .js extension in relative imports even though you're importing .ts files

[lbroudoux]

These are actually recommendations from testcontainers (see https://github.com/testcontainers/testcontainers-node/blob/main/tsconfig.base.json). What is the impact of changing these?

[gregersrygg]

node10: Formerly known as node, this is the unfortunate default when module is set to commonjs. It’s a pretty good model of Node.js versions older than v12, and sometimes it’s a passable approximation of how most bundlers do module resolution. It supports looking up packages from node_modules, loading directory index.js files, and omitting .js extensions in relative module specifiers. Because Node.js v12 introduced different module resolution rules for ES modules, though, it’s a very bad model of modern versions of Node.js. It should not be used for new projects.
node16: This is the counterpart of --module node16 and --module node18 and is set by default with that module setting. Node.js v12 and later support both ESM and CJS, each of which uses its own module resolution algorithm. In Node.js, module specifiers in import statements and dynamic import() calls are not allowed to omit file extensions or /index.js suffixes, while module specifiers in require calls are. This module resolution mode understands and enforces this restriction where necessary, as determined by the module format detection rules instated by --module node16/node18. (For node16 and nodenext, module and moduleResolution go hand-in-hand: setting one to node16 or nodenext while setting the other to something else is an error.)
nodenext: Currently identical to node16, this is the counterpart of --module nodenext and is set by default with that module setting. It’s intended to be a forward-looking mode that will support new Node.js module resolution features as they’re added.
https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution

When looking at the testcontainers project, I'm afraid they're having the same issue. Also looks like they attempted to convert to ESM a while back, but closed the PR without any reasoning: testcontainers/testcontainers-node#406

I don't have much time to look into this, and it's not a critical issue. We can just continue to use an old version, but it would be nice for others if you either revert the module change in package.json or continue with the change to a proper ESM module. The module type in package.json is looked up at runtime, and type "module" tells the JS host (Node.js) that the javascript in this project is using import/export and not require. See more about how it works here:
https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution

[gregersrygg]

Sorry, I didn't get time to clean up the PR yesterday, so I left it as a draft. Didn't intend to also include the whitespace formatting that my editor modified automatically. I will do a force-push to avoid those unrelated changes.

However, I didn't see the regression as the microcks-testcontainers-node-nest-demois still working with themicrocks-testcontainers` used in Typescript (as see here for example). Does it mean that we're doing things the wrong way in this demo repository?

I don't think there's a right and wrong here, but I think the reason this didn't fail in that demo project is because the projects tsconfig.json had configured "module": "commonjs". If you update the tsconfig.json to use ESM and nodenext module resolution (import ... instead of require(...), you'll see the same errors as we got:

    "module": "nodenext",
    "moduleResolution": "nodenext",

So the change to module in package.json probably only affects those that used nodenext already, but it's a breaking change for them.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 30 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. Microcks is a Cloud Native Computing Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

[utamori]

any update ?

[aleksanderson]

Hi all,
Any update on this one?

[gregersrygg]

@lbroudoux Others seem to have the same issue. As I mentioned earlier; the current module type is incorrect, so the module type should be reverted or the project updated to use import/export.

[lbroudoux]

@lbroudoux Others seem to have the same issue. As I mentioned earlier; the current module type is incorrect, so the module type should be reverted or the project updated to use import/export.

Sorry, I was super busy these last weeks. I need to dive deeper in this one. I'll probably find some time this week.

[lbroudoux]

Shouldn't this one be in devDependencies?

[lbroudoux]

Hey @gregersrygg, @utamori, @aleksanderson and others who want to participate in the discussion!

I spent some time today looking at this. It's a bit hard for me to understand all these packaging subtleties as JavaScript/Typescript is far from being my main focus... However, I think I succeeded in grasping the main parts.

I did two experiments that led to stable stuff, but with different consequences for project who'd like to consume the lib. Let me explain what I saw and, please, correct me if I'm wrong on some assumptions or conclusion.

1st: Stick with "type": "module" in package.json

I applied the changes suggested in the PR, also updated Node version + a bunch of libraries and replaced Jest with Vitest, and everything seems to work. 🎉 However, as a consequence, projects that consumed the project have to update their tsconfig.json to now include "moduleResolution": "nodenext", so that they can understand the exports done in our index.js

From what I saw, this differs from the testcontainers lib itself that still produces modules without the "type": "module".

What I don't know here is:

• Whether updating the module resolution is a big deal or not for most project?
• Where is the JS/TS community regarding this adoption?
• Is it still possible to use our lib from pure JavaScript projects in that case?

2nd: Remove "type": "module" in package.json

This was not possible previously because we were using Jest, which brings a lot of incompatibilities. As stated above, I updated Node version + a bunch of libraries and replaced Jest with Vitest, and everything seems to work. 🎉. This has no impact on our existing microcks-testcontainers-node-nest-demo demo that specifies no module resolution.

From what I saw, this aligns with the testcontainers lib itself.

What I don't know here is:

• Are there any usability issues going this way? Are things more difficult to use/import/configure in that case?
• Where is the JS/TS community regarding the move to pure ESM modules?
• Is it still possible to use our lib from pure JavaScript projects in that case?

Wrap-up

Please help me find how to go forward on this? My preference goes to doing things in the cleanest way possible, trying not to evict the users who may still deal with legacy projects that have not done the latest move -- even if I think that if they have an interest in testcontainers, they probably are on recent projects but...

Any input, feedback, or personal experience is much appreciated here on how to go forward! It's now just a matter of making the decision on where to go next. Thanks!

[gregersrygg]

@lbroudoux Thank you for following up!

Reverting the change (2nd option) is probably the safest, and can be released as a patch release. I suggest doing this even if you want to fully convert the project to ESM. Bundlers typically handle converting between CJS/ESM as long as it's type in package.json is correct.

Where is the JS/TS community regarding the move to pure ESM modules?

Hard to say for sure since many packages on npmjs are old and not maintained. ESM adoption rate is growing, but still not as big as CommonJS. An article and a tool that can analyze a projects dependencies here: https://antfu.me/posts/move-on-to-esm-only

Is it still possible to use our lib from pure JavaScript projects in that case?

The type in package.json defines what the source code of that project is using (as packaged JavaScript). So reverting the type change should be completely safe, as it's incorrect currently.

The 1st option (keep module) is optional in my opinion and not urgent, but it should be a major release because of the breaking change. Sindre Sørhus (author/maintainer of 1000+ packages) has written a guide on how to convert a project to use ESM that you can link to in the readme/release notes. Consumers of the library that don't want to switch to ESM can then keep using the older version until they're ready to switch.

[lbroudoux]

Thanks @gregersrygg for your comment.

The 1st option (keep module) is optional in my opinion and not urgent, but it should be a major release because of the breaking change.

Totally agree. Let's keep watching this topic and plan this maybe for a 0.4.0 release or whatever.

Reverting the change (2nd option) is probably the safest, and can be released as a patch release.

Sounds like a plan! Is releasing as a patch a specific process on NPM? I am ignorant of this topic. Hints or links are welcomed!

@utamori: You seem to be ok with this one. Could you confirm?

[gregersrygg]

Totally agree. Let's keep watching this topic and plan this maybe for a 0.4.0 release or whatever.

I would categorize this as a bug fix according to Semantic Versioning which is common for NPM packages:

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes
MINOR version when you add functionality in a backward compatible manner
PATCH version when you make backward compatible bug fixes

In other words, only the last number should increase because there are no new features and it's a backwards compatible bug fix.

Sounds like a plan! Is releasing as a patch a specific process on NPM? I am ignorant of this topic. Hints or links are welcomed!

Nothing special. Just that the last number in the version is bumped.

When changing the type to module and fully use ESM in the project would be categorized as an incompatible API change because it requires at least some users to update their project in able to use that new version, and hence it should be a major release (bump first number in the version).

[lbroudoux]

Hi all! I've made the revert changes and it's now tracked by #127. I expect to do the release by the beginning of next week. As a consequence, I am closing this issue.

Thanks again @gregersrygg for the explanations and the patience you demonstrated! Feel free to reach out for any issues or improvements ideas! We're looking for contributors to help us! 😉