-
-
Notifications
You must be signed in to change notification settings - Fork 525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for ESM config files #987
Conversation
Failing unit test is indeed not your fault, but is something we should look into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to look good. Will wait a few days for @corevo to respond to this as well. Can you change the title of the PR so we can squash this when merging?
|
||
// mimics what `import()` would return for | ||
// cjs modules. | ||
return { default: require(modulePath) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that the module only has a default export, and is not compatible with named exports.
For the purposes of the config that's fine, because the config is exported on the default export when is ESM compatible, but this is not true for all cases.
Which makes me feel like it would be better to invoke supportsDynamicImport
in config-helper.js
and either use the import helper, or straight up require.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand what you mean.
If the user is using named exports then the platform is compatible with ESM and import()
will be used, which supports named exports.
If they're using CJS, there are no named-exports.
Native import()
of a CJS file already returns { default: module.exports }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you mean, I think I didn't fully understand what you were saying in #982
But basically you wanted to write a node 10 compatible importModule
function, while I was under the impression that you were going to drop support for node 10.
EDIT: by you, I mean the sequelize team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll indeed drop Node 10 for v7, but I don't think we want to wait with fixing this until the core sequelize package is ready for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, makes sense, I think we can resolve this then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, so that's an approval from you? Then I think we can merge and set up a new release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly :) The point was to replicate import()
using require
, the code that uses import-helper
is then responsible for properly using the module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point was to replicate import() using require
For sequelize v7, will this solution be dropped and using the native import
be supported instead? Along with .sequelizerc
no longer having module.exports
? Should I open an issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the next major will drop support for node 10 so import()
will always be used.
Supporting ESM in .sequelizerc
could already be done in this version though. It should be a small change too.
Simply need to make this line use the new import helper
Line 9 in 54ec29d
? JSON.parse(JSON.stringify(require(rcFileResolved))) |
.sequelizerc
+ .sequelizerc.json
+ .sequelizerc.cjs
+ .sequelizerc.mjs
+ .sequelizerc.js
.
(feel free to open an issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've openeded #989 , which is only for v7.
I think current version is fine as is, would prefer focusing on v7 since sequelize v7 is almost ready.
@ephys I'll leave the merging up to you |
Here we go 🎉 |
🎉 This PR is included in version 6.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Can https://sequelize.org/master/manual/migrations.html#dynamic-configuration be used with ESM now that #987 has been merged? According to docs, this means
|
The documentation PR hasn't been merged yet but it should work (sequelize/sequelize#13669) |
Thanks. That PR doesn't have ESM examples for config.js / .sequelize.rc. I don't know how to convert existing format to ESM. |
config.js can be ESM the same way other ESM files are handled in Node:
and the configuration should be the default export. I'll update the PR with this comment. Is there any other information you need to add to the doc? |
ESM with sequelize has been a little confusing for me. I tried going through the issues in sequelize and sequelize-cli and my understanding is that we can write ESM code with sequelize but not for migrations using the CLI. The |
I agree that it's confusing. It's mainly due to us not having enough bandwidth available right now to improve this project as we're hard at work on the main repository What I'd like to do is rewrite this project as esm and add native support for esm everywhere, as a major release that drops node < 12. |
Sounds good to me. I think every node project should ideally support/adopt ESM as standard. It's the way forward. I mean I rewriting an sdk that uses sequelize at work to ESM modules. That's how I ended up here. NB: sorry to ping all the people in this closed PR. |
It seems that this PR broke the usage of TypeScript with sequelize-cli 6.4.0. $ <project>/node_modules/.bin/sequelize db:migrate
Sequelize CLI [Node: 16.14.0, CLI: 6.4.0, ORM: 6.20.1]
ERROR: Error reading "sequelize/config.ts". Error: TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for <project>/sequelize/config.ts |
Description of change
Continuation of PR #982
This PR uses feature-detection to detect whether we can use
import()
to import the configuration file.It also fixes the babel config to properly transpile
import-helper.js
for node 10.Documentation PR is still sequelize/sequelize#13669
Unit tests fail but I think it's unrelated to this PR. Need to investigate.
Also pinging @corevo :)