-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update module resolution to NodeNext #10620
Conversation
|
✅ Deploy Preview for apollo-client-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
@TrevinAvery It seems like many (44) tests are failing because of these changes. Can you investigate further? |
Yes. I've been looking into it. For some reason the tests pass on my
machine, but not in the PR. I'm not sure of a better way to test the
circleci integration than just making a PR. So this might be a little noisy
as I try different things.
…On Wed, Mar 8, 2023, 10:25 AM Ben Newman ***@***.***> wrote:
@TrevinAvery <https://github.com/TrevinAvery> It seems like many (44)
tests are failing because of these changes. Can you investigate further?
—
Reply to this email directly, view it on GitHub
<#10620 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGBCSFQTXYHSFZUD6KODDNLW3DFHZANCNFSM6AAAAAAVONMSI4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Unfortunately, adding an We can only ship something like that in a major - and that won't happen in the next few months. The only compromise I could think of here would be adding a Could you try if that angle would also solve your problems? |
@phryneas My understanding is that if a bundler doesn't know how to use Also, since using
To my understanding, and what I've read in the node.js docs, this should cover all the bases. Questions:
|
@TrevinAvery I would have to dig in years of twitter threads to pull out specific examples, but there are reasons. Once in a while, someone tries it and things start breaking left and right. Every time in a fun, new and unexpected way. I believe one of the last cases was As a result, in the library author community, we widely see adding I'll still try to answer the questions:
|
Ok, so moving forward, is there a path for this change? Can we put this on
the road map for the next major release, or should I discard the PR?
…On Wed, Mar 15, 2023, 1:51 AM Lenz Weber-Tronic ***@***.***> wrote:
@TrevinAvery <https://github.com/TrevinAvery> I would have to dig in
years of twitter threads to pull out specific examples, but there are
*reasons*. Once in a while, someone tries it and things start breaking
left and right. Every time in a fun, new and unexpected way. I believe one
of the last cases was immer <immerjs/immer#937>
that added exports in a patch release.
That's fine in a major, it's not in a minor.
As a result, in the library author community, we widely see adding exports
as a reason to put out a new major version.
I'll still try to answer the questions:
1. I would be most afraid of a bundler that worked without exports
after the change recognizes exports, but handles it in a slightly
different way than before.
2. Unfortunately not, so we have to care about breaking bundlers we
don't even know about. (Which, again, would be fine in a major and not fine
in a minor)
3. You're not really misunderstanding anything, everybody looks at it
the way you do first because it seems easy in the docs. Then you release
something with enough users, and reality gets back to you. This is
generally an absolutely horrifying minefield.
Just as an example of the complexity involved here, here is a fun
little twitter thread about the fun one of the TypeScript maintainers has
to deal with: https://twitter.com/atcb/status/1634653474041503744
—
Reply to this email directly, view it on GitHub
<#10620 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGBCSFSHXVCXOF6AZBASPVDW4F7KHANCNFSM6AAAAAAVONMSI4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
My suggestion would be that you evaluate a Revisiting the packaging and bundling is definitely on the Roadmap for the next release - so in that regards, there is nothing to be done :) |
While I really appreciate the PR, I believe except for the When we get to the point of adding an As a result (and because rebasing all of the work in here is probably going to be a nightmare), I will close this PR for now and will revisit the topic when we target a new major version. I hope that's okay with you @TrevinAvery. I want to repeat myself here - I really appreciate the work you put into this! |
@TrevinAvery For what it's worth, I have a temporary workaround that can be used with |
Checklist:
This PR addresses #9890 and this feature request by updating the entire package to use
NodeNext
module resolution and add theexports
field to allpackage.json
files in the distribution.There are two major changes that had to be made:
Updating all relative imports.
The
NodeNext
module resolution enforces the latest ESM requirements, which includes using the full file name for import statements. This means that for importing a file, the import must end in.js
, and for importing a directory, theindex.js
file must be named explicitly. All changes in thesrc
anddocs
directories should be exclusively one of these two changes. The files in theconfig
directory also needed these changes, but required additional work as well.Updating configuration files to TypeScript.
There was a bit of complexity in getting the package to build with the new imports while also getting the tests to pass.
entryPoints
unless it was also TypeScript.entryPoints
file, so it also had to be moved to TypeScript.config/tsconfig.json
file and modify the one at the root of the package because rollup does not have an option to configure whichtsconfig.json
file to use.tsconfig.build.json
and configured Jest andtsc
to use that file.require('*/index.js')
torequire('*/<bundleName>.cjs')
.exports
field to eachpackage.json
file in theprepareDist.ts
file.This change should be fully backwards compatible, with no noticeable change to its external API, with the only exception that it should be correctly imported in any project using
NodeNext
. For this reason, I have not included a change set. If this is needed, I'd be happy to add one.Since this is such a large code change, I am looking for some general feedback and would be happy to address anything I may have missed.