Migrate to ESM-first

[SmashingQuasar]

Hey!

So this may seem like a feature request, but it actually falls under the bug category in my opinion.

As of today, umzug is written and built in CJS.
The problem is, this breaks applications written in pure ESM because umzug is using a dynamic require.
This problem is currently affecting several ORMs (including sequelize).

I think migrating to ESM-first is necessity since ESM supports importing any kind of module unlike CJS.

I am willing to write a PR to that effect if reviewers are available to check it.
This will obviously create many changes within this dependency. It should not, however, create any breaking change.
Let me know if you are willing to migrate and if you want a PR implementing this change.

[ephys]

I'm personally very pro-esm, and umzug is one of those libraries I think could be fully converted to ESM (dropping support for CJS) without being a huge problem because it's often used indirectly.

The sequelize cli will be undergoing a complete rewrite in ESM, so umzug going ESM would not be a problem for the cli team.

That being said, umzug can be used with ESM, but you need a custom resolver to replace the default resolver: https://github.com/sequelize/umzug/blob/586675894c3b099bb50ad2f07e46d89277ee29bb/examples/2.es-modules/umzug.mjs

I would argue that, at a minimum, the default resolver should be changed to use a dynamic import() instead of a dynamic require().

[mmkal]

I don't want to drop commonjs support, but yes, I agree that esm should work out of the box, although as @ephys mentioned it does work already with a little extra work. For the current major version, it would probably need to be limited to looking for a .mjs extension.

In general, I want to make sure umzug stays framework agnostic and unopinionated - sequelize is an important consumer but not the only one.

[ephys]

sequelize is an important consumer but not the only one.

Absolutely, please take my comment as coming from a user of umzug that pushes for ESM but is unrelated to umzug even if we're in the same org

[SmashingQuasar]

I don't want to drop commonjs support, but yes, I agree that esm should work out of the box, although as @ephys mentioned it does work already with a little extra work. For the current major version, it would probably need to be limited to looking for a .mjs extension.

In general, I want to make sure umzug stays framework agnostic and unopinionated - sequelize is an important consumer but not the only one.

CJS is natively supported by ESM, the opposite is not true. This is due to the asynchronous nature of ESM.
If you want umzug to be agnostic and unopinionated, then it should be written in ESM.
Moving on to ESM will not drop CJS support, it will simply make ESM work so I believe that migrating to ESM would meet your expectations.

[ephys]

By "drop support for CJS", what we mean is that CJS codebases won't be able to require() this library anymore, not that this library won't be able to import CJS migration files.

Dynamic import is available to both CJS and ESM, so we can support ESM migration files without dropping support for being imported by CJS codebases by replacing the dynamic require() by a dynamic import(), like here

But doing this is indeed a breaking change, because hooking import (for things like typescript support) uses the loader API instead of hooking require

[SmashingQuasar]

I see, you mean that umzug is actually using hooks on require to achieve it's purpose?

[mmkal]

I would like umzug to take a side in the holy war of support-commonjs vs don't-support-commonjs, but not in the holy war of esm vs commonjs - esm is almost certainly the way of the future, but some OSS maintainers don't want to support developers who live in the present and are stuck with commonjs. I do want to support such developers, partly because I'm one of them in my day job.

It shouldn't be too hard to support everything. We can start using microbundle or similar to compile the package, so it has a commonjs and an esm import option. And we can pretty easily add support for .mjs in the current major version. If someone wants to open PR(s) for either of these things, feel free to. Otherwise I will aim to do it some time in July.

In the next major, we could default to assuming .js is esm instead of commonjs and look for .cjs for commonjs. But I'm in no particular rush to make that change.

[ephys]

I very, very strongly recommend that you do not provide separate builds for esm and commonjs. That inevitably leads to issues as both version end up loaded in the same application but are incompatible with each other. I opened yet another bug report about this today in Apollo server.

If you want to support being imported by both cjs and esm, write the library in cjs and provide an esm wrapper that declares the named exports. If you need a real life example, look at how Sequelize 7 handles this.

You also don't need to assume anything regarding whether .js is esm or cjs. Dynamic import() will import either correctly based on the extension and the package.json. All you need to do is to import the migration files with import instead of require, no extension check necessary.

I see, you mean that umzug is actually using hooks on require to achieve it's purpose?

Users could have require hooks to run the file through something like babel or ts-node at the time it gets loaded

[mmkal]

If you want to support being imported by both cjs and esm, write the library in cjs and provide an esm wrapper that declares the named exports. If you need a real life example, look at how Sequelize 7 handles this.

This is pretty close to the status quo already. Despite it being mentioned earlier in this thread I forgot that we have an example of using esm, both for the Umzug instance and the migration files themselves. It requires only a tiny bit of extra work, and even supports mixed .cjs and .mjs migrations. https://github.com/sequelize/umzug/tree/main/examples/2.es-modules

I see this issue as a feature request, to make the above scenario boilerplate free (ideally, no need for createRequire, and no need to write a custom resolver).

I very, very strongly recommend that you do not provide separate builds for esm and commonjs. That inevitably leads to issues as both version end up loaded in the same application but are incompatible with each other. I opened yet another bug report about this today in Apollo server.

You are probably right - there may be no need for microbundle (partly because umzug in general doesn't need "bundling" anyway), and a wrapper.mjs will work. I will also test out just using import(...) in either case.

(Re the issue you opened, I'm guessing you're referring to this one? This is helpful, thank you!)

[SmashingQuasar]

I very, very strongly recommend that you do not provide separate builds for esm and commonjs. That inevitably leads to issues as both version end up loaded in the same application but are incompatible with each other. I opened yet another bug report about this today in Apollo server.

I have been bundling my code and libraries at work for both ESM and CJS for years even on projects with hundreds of files without having any issue. May I suggest that the libraries you are having issues with may not know how to bundle ESM and CJS in different exports?

Users could have require hooks to run the file through something like babel or ts-node at the time it gets loaded

It is impossible to support every niche use case. Production should be the target and I sincerely, deeply, profundly hope that nobody is running ts-node in production. I know people are unfortunately.

If you want to support being imported by both cjs and esm, write the library in cjs and provide an esm wrapper that declares the named exports. If you need a real life example, look at how Sequelize 7 handles this.

I don't believe that to be a proper solution. This is how we end up having so many libraries still written in CJS in 2023 because people do not want to move forward.

I'll let you people decide how you want to handle this situation as we seem do have relatively opposite views on the matter. If you want help to migrate to ESM, I'll gladly help you. 👍

[ephys]

I have been bundling my code and libraries at work for both ESM and CJS for years even on projects with hundreds of files without having any issue. May I suggest that the libraries you are having issues with may not know how to bundle ESM and CJS in different exports?

Please see https://nodejs.org/api/packages.html#dual-commonjses-module-packages, and apollographql/apollo-server#7625. This is a very real issue, you just haven't encountered it yet.

It is impossible to support every niche use case.

That's still a breaking change, and should require a major release

I don't believe that to be a proper solution. This is how we end up having so many libraries still written in CJS in 2023 because people do not want to move forward.

If we want to support both CJS and ESM, it is our only option. I don't understand why it bothers you that the library uses CJS internally if it has no observable impact on you as an ESM user. The only people it impacts are the ones maintaining the library

[JSteunou]

Searching on current status of ESM support at umzug I found this discussion. Just dropping my 2 cents: in a full ESM project, using top level await, running migrations with umzug fails because it is CJS.

ERROR: Top-level await is currently not supported with the "cjs" output format

That is a bummer in 2023. One vote for ESM.

[SmashingQuasar]

I have been bundling my code and libraries at work for both ESM and CJS for years even on projects with hundreds of files without having any issue. May I suggest that the libraries you are having issues with may not know how to bundle ESM and CJS in different exports?

Please see https://nodejs.org/api/packages.html#dual-commonjses-module-packages, and apollographql/apollo-server#7625. This is a very real issue, you just haven't encountered it yet.

Sorry, I forgot to answer you when I read your response.
To me, this problem is a consequence of having way too many dependencies. Nowadays people will install 80+ dependencies for their project. It's no wonder there are issues. If people do that, then in my opinion, it's not really a Node.JS issue but a development and applicative architecture issue.

It is impossible to support every niche use case.

That's still a breaking change, and should require a major release

I see no issue with major releases.

I don't believe that to be a proper solution. This is how we end up having so many libraries still written in CJS in 2023 because people do not want to move forward.

If we want to support both CJS and ESM, it is our only option. I don't understand why it bothers you that the library uses CJS internally if it has no observable impact on you as an ESM user. The only people it impacts are the ones maintaining the library

CJS is more a reflection of a refusal of change. Every Node.JS developers knows (or is lying to themselves) that CJS will eventually disappear. Maybe not in the following years but it is bound to disappear. I will never understand engineers or anyone involved in science that simply pushes back against progress. Aren't we supposed to be the one pushing progress? To me, by hard-rooted in CJS and refusing to change is not a good sign for the future of the library. It's not like ESM was released yesterday, it's been over 7 years now. I just want to emphasize that I am not meaning any harm or offense here. I am just voicing my disappointment in the JS community overall that happens to be somewhat reflected here.

[github-actions]

Released (as beta) in v3.5.0-0.

[mmkal]

@SmashingQuasar I am hopeful that #613 is a satisfactory solution to this.

It's worth noting that it's only possible for #613 to solve the use-case here without breaking others' use-cases by not migrating to ESM-first. (Hence the title "ESM support" rather than "ESM migration"). It's worth reading @ljharb's comments in eslint/rfcs#88 which IMO articulate why well. As the maintainer of umzug (a package) - I think sticking with CJS is the right call. Nothing about that decision prevents you, a user of umzug (and presumably maintainer of an application) from modernizing to ESM. That eslint RFC also covers the (non-)benefit of applying "type": "module" to the package.json which I opted not to do either.

@ljharb sorry for the @ mention out of the blue, no need to respond!

[github-actions]

Released in v3.5.0.