Thanks for running the checks :)

Does the CI give any detail as to why the lint check failed? Replicating the steps locally gives me no errors

I think it was a temporary issue with GitHub Actions, @sdepold can you re-run the checks?

Assuming we eventually have full support for TS, will this still be needed? I'm somehow reluctant to merge since we would have to keep both index files in sync, right?!

Once you have a full TS implem that is ESM compatible (the only impact is I think that you need to specify the file extension in imports, and not use directory imports), then you should be able to transpile to both CommonJS and ESM.

Both index files would have to be kept in sync yes, this is more of a temporary solution that works today and can be removed once the TS migration is complete

Edit: transpiling for both is actually a bad idea, see #13689 (comment)

@Keimeno @allawesome497 --^

Opinion?

I think I would rather try to avoid merging this.

Will take a deeper look into this in the evening

I understand that having to maintain two exports file is annoying. If this does not get merged, do you have an estimate regarding how long it would take to fully migrate to an ESM compatible implementation?

More and more libraries are moving to native-esm only, forcing apps to migrate too and while we can import commonjs, every import must be changed from this

to this

(as well as having to split uses between the type and the value, making it incompatible with TS's emitDecoratorMetadata).

This temporary patch would remove the need for users to make the migration, and this patch can be removed once the TS migration is complete. (see #13689 (comment))

Edit: extra thought regarding this:

This method has another advantage over transpiling for both versions: A CommonJS app that uses both import('sequelize') and require('sequelize') will import the same instance. Whereas transpiling for both versions will result in two different instances.

(sequelize-esm in the above demo is the same wrapper as the one I put in this PR)

And regarding maintaining both files: I can add a test that ensures the ESM export has the same keys as the CJS export.

I think I would rather try to avoid merging this.

I'd also opt to avoid merging this, I think its better to wait until we can convert this to TS.

If we want to get this working for mjs ASAP I think something like this should work:

// lib/sequelize.js

module.exports = Sequelize;
// We're changing the `Sequelize` object here, its just more readable as 
// module.exports + that matches what we already have in `lib/sequelize.js`
module.exports.Sequelize = Sequelize;
module.exports.default = Sequelize;
module.exports.__esModule = true; // tell ES modules that this is an esmodule.

But that is not how native ES modules work. Only bundlers respect the __esModule property.
When a module imports a commonjs script, only the default import is available (and equal to module.exports).

There are only 3 ways to support ES Modules:

Option 1: Convert the whole codebase to ES Modules and drop CommonJS support

cons:

• This is a huge breaking change, a lot of apps are written using CommonJS and CommonJS can only import ES Modules using Dynamic Import (import(x): Promise)
• It's a lot of work: You need to make sure every import is compatible with ESM. That means always specifying the file extension, and being careful when importing third-party libraries as CommonJS modules do not support named exports (you can only import module.exports as the default export)

Option 2: Convert the whole codebase to ES Modules and transpile to CommonJS

cons:

• Similarly, a lot of work
• doing import('sequelize') and require('sequelize') will return two different instances of Sequelize as import() will import the module version and require the script. This will cause issues. Especially since import() is available to CommonJS

Option 3: Keep the codebase as CommonJS and include a module that re-exports the CommonJS module using named exports

cons:

• You need to maintain an export file (but that's what unit tests are for)

Closing note: Node does have some static analysis for exposing named exports from CommonJS modules but it should not be relied upon, it's also not available for node 12.

Alternatively, once we finish the migration to TypeScript we should be able to configure the package to allow us to build for both nodejs and cjs, as esbuild supports this.

We'd probably want to implement a clone of the index file though, since lib/seqeulize.js currently relies on module.exports (the export is the class Sequelize, but Transaction, DataTypes, etc are also exported).

(I believe we can include both an index.js and an index.mjs, though I'm not 100% sure if this is needed)

Transpiling to both CommonJS and ESM is exactly the option 2 I described and it is going to cause huge issues to CommonJS users:

doing import('sequelize') and require('sequelize') will return two different instances of Sequelize as import() will import the module version and require the script. This will cause issues. Especially since import() is available to CommonJS

You can include both index.js and index.mjs, that's exactly what this PR is doing. But it's doing it in a way that ensures all values exported by both CJS and ESM are the same instances.

Sorry for the ping but any update on this @Keimeno @sdepold? I think #13689 (comment) explains why this PR is the only way to support ESM named exports and CommonJS at the same time without causing weird bugs

doing import('sequelize') and require('sequelize') will return two different instances of Sequelize as import() will import the module version and require the script. This will cause issues.

What kinds of issues? I can't see why a module would be partially CJS and partially MJS at runtime.

I think its also better to have a full ESM module for performance & security rather than having a mixed source.

Especially since import() is available to CommonJS

In NodeJS v16.7.0 I get SyntaxError: Cannot use import statement outside a module.

And similarly, in NodeJS v16.7.0 I can't use require from a mjs module without using node's module api to create a require function, ie:

import Module from 'module';

const require = Module.createRequire('/full/path/to/file'); // __filename also isn't available in .mjs files otherwise that's what we'd use here.
const Sequelize = require('sequelize');

I would be fine with supporting esm in the short term with this, I'd rather have something than nothing for anyone who wants to use ESM.

However I think that in the long term it'd be best to have both esm and cjs compile targets, as the source will be esm and I'd like to keep the transpiled code as close to its native runtime as possible.

@ephys this change doesn't account for esbuild and dist/, can you modify the build.js script to account for this? I believe a simple cp index.mjs dist/ (translated to use node's fs builtin if possible) should suffice.

In NodeJS v16.7.0 I get SyntaxError: Cannot use import statement outside a module.

You're mistaking the static import with the dynamic import. As specified here, dynamic imports are available to CommonJS. Static imports are not.

Plus, as you said yourself, ESM could also access require through module.createRequire and load two different versions of Sequelize without realizing that they are different.

What kinds of issues? I can't see why a module would be partially CJS and partially MJS at runtime.

Most common case: by using any library written in CommonJS that depends Sequelize. They will all break for ESM users.

Say your write your app in full native esm.
Your app uses a library written in CommonJS, which itself depends on Sequelize (say https://github.com/ephys/sequelize-cursor-pagination).

Your app is therefore going to do this:

esm app → `import 'sequelize'` (imports the ESM version of Sequelize)
        → `import '@ephys/sequelize-find-by-cursor'` → `require('sequelize')` (imports the CommonJS version of Sequelize)

@ephys/sequelize-find-by-cursor has a method findByCursor which takes a Sequelize Model as a parameter.

For type safety, this method checks whether the parameter it received is indeed a sequelize model like so:

Using the solution of providing two completely separate transpilation outputs, that is going to return false as there will be two different Model classes. Whereas Solution 3 exposes the same Model class to both CJS and ESM, so this type safety check will pass.

import SequelizePkg from 'sequelize';

(async () => {
  console.log((await import('sequelize')).default === SequelizePkg);
})();

const SequelizePkg = require('sequelize');

(async () => {
  console.log((await import('sequelize')).default === SequelizePkg);
})();

I think its also better to have a full ESM module for performance & security rather than having a mixed source.

What extra security are you speaking of?
As for performance, the tiny improvement to startup time this would bring is definitely not worth the cost of introducing weird bugs like Sequelize from import() not being equal to Sequelize from require().

By the way, please note that the code that's currently being written for the TypeScript migration is not valid ESM.
To support ESM, the source code needs to specify the file extension when it imports a file. The TypeScript compiler is not going to add them: https://nodejs.org/api/esm.html#mandatory-file-extensions

This change doesn't account for dist/, it'll need to be modified to copy the index.mjs file to dist/ (I don't think any esbuild tanspiling is necessary here).

Will do

I have added a v7 branch now and we could start removing support for node 10 in that branch since we planned to do that anyway. Should we target that branch instead of main maybe?

I think we can land this in v6, node 10 users won't be able to import this file anyway so I can just skip the test for node 10.

The test has been disabled on node 10, only thing left to resolve is decide whether we handle the typing issues the test unearthed in this PR (#13689 (comment))

Let's do that in a separate PR if you don't mind and we will merge this PR after that one

Here we go :) #13751

looks promising

I assume some update is needed now?

Yes, just need to tell the test that Sequelize.db2 is not an export. Should be good now :)

Woop

🥳
Thank you for merging :)

I'll release this as part of our beta releases in a moment :)

🎉 This PR is included in version 6.12.0-beta.3 🎉

The release is available on:

• npm package (@v6-beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

Nice work on this! @ephys
I just thought it might be nice to include this to the docs somewhere, could you maybe take a look into that soon?

🎉 This PR is included in version 6.12.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀