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
fix: allow deep imports #13795
fix: allow deep imports #13795
Conversation
While working on this PR I realized the TypeScript migration introduced a breaking change: Before the TS Migration, importing After the TS Migration, it will usually fail as it will, most of the time, try to import a TS file. This PR also fixes that by mapping the As such, I'd like to add Note: it only fixes it for node >= 12 as node 10 does not support export mapping. I have no way of fixing it for node 10 without reverting the whole TypeScript migration and delaying it to v7. :/ I think this unexpected breakage is a good argument for forbidding importing |
Completely agreed on the fact that people would better not import anything from the package other than the main entry point. |
I'll PR v7 to completely forbid it once this is merged :) |
Thanks a bunch. I think I'll just release a new version straight away or was there anything else we need to fix? |
@ephys Would you mind pinging me on Slack? |
@sdepold Yep! |
🎉 This PR is included in version 6.12.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* fix: allow deep imports * fix: fix export declarations * test: fix deep-exports test Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
Pull Request Checklist
Please make sure to review and check all of these items:
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description Of Change
Fixes #13787
The issue was that once
exports
is defined in package.json, node uses it as the source of truth for all valid exports of a package.I've configured
exports
to replicate how the imports worked in v6.I also had to remove the alias in
registerEsbuild.js
as it was redundant withexports
.It also matches reality better as the tests use the same export definition as the published build. :)
I recommend changing the export declaration for V7, see below
Breaking changes recommendations for V7
In v7, we could change the export definition to
This way, instead of importing
sequelize/lib/model.js
, users could importsequelize/model
.If we still want users to specify
/lib
in v7, we can use this one instead[recommended] If we want to prevent users from importing
/lib
which could result in unexpected breaking changes, we could instead use this one which only allows importingsequelize
&sequelize/package.json