-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Remove deprecated wildcard folder mapping #23256
Conversation
packages/react-dom/package.json
Outdated
"./package.json": "./package.json", | ||
"./": "./" | ||
"./unstable_testing": "./unstable_testing.js", | ||
"./umd/*": "./umd/*", |
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 kept the UMD bundles reachable for now until we make a final decision on when to remove them
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.
So, my thinking about the umd bundles is that even if you do use them, you typically don't use them through a require implementation that goes through exports. Like you could technically require them but you're not supposed to. I think we can remove these and then if we get a report that's legit maybe add them back in a patch/minor?
e667998
to
2fb3805
Compare
packages/react-dom/package.json
Outdated
@@ -50,8 +50,10 @@ | |||
"./profiling": "./profiling.js", | |||
"./test-utils": "./test-utils.js", | |||
"./unstable_testing": "./unstable_testing.js", | |||
"./package.json": "./package.json", | |||
"./": "./" | |||
"./testing": "./testing.js", |
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 is an experimental package. It got accidentally added to the @next
release channel. We don't intend to release to 18 stable. I'll remove it in a follow-up.
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.
Will be fixed by @sebmarkbage in #23258
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.
If you're rebased on mine, can you remove this now?
Comparing: 274b9fb...c281dd9 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
2fb3805
to
05dd99b
Compare
4db18bc
to
ee66a87
Compare
@@ -27,7 +27,9 @@ | |||
"./package.json": "./package.json", | |||
"./jsx-runtime": "./jsx-runtime.js", | |||
"./jsx-dev-runtime": "./jsx-dev-runtime.js", | |||
"./": "./" | |||
"./umd/*": "./umd/*", | |||
"./unstable-shared-subset": "./unstable-shared-subset.js" |
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.
Instead of exposing this, can we instead just use the deep linking into /src/
for the bundle? Because that way it's not exposed upon publishing.
Also, deleting all ./src/*
entries after bundling is easier than any arbitrary name.
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.
Yeah that sounds good
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 added the name
field to bundles for this purpose since otherwise they'd get the deep link name. If I had that at the time I don't think I would've added this top level entry point.
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 going to open a separate PR for this so I can get it reviewed
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.
What's you plan for the ./src/*
entry in published package.json? Just leave it in even though the files are not there or remove post-build?
ee66a87
to
7cf1e59
Compare
Planning to remove it post-build since the Rollup script still needs it |
Node v16 deprecated the use of trailing "/" to define subpath folder mappings in the "exports" field of package.json. The recommendation is to explicitly list all our exports. We already do that for all our public modules. I believe the only reason we have a wildcard pattern is because our package.json files are also used at build time (by Rollup) to resolve internal source modules that don't appear in the final npm artifact. Changing trailing "/" to "/*" fixes the warnings. See https://nodejs.org/api/packages.html#subpath-patterns for more info. Since the wildcard pattern only exists so our build script has access to internal at build time, I've scoped the wildcard to "/src/*". Because our public modules are located outside the "src" directory, this means deep imports of our modules will no longer work: only packages that are listed in the "exports" field. The only two affected packages are react-dom and react. We need to be sure that all our public modules are still reachable. I audited the exports by comparing the entries to the "files" field in package.json, which represents a complete list of the files that are included in the final release artifact. At some point, we should add an e2e packaging test to prevent regressions; for now, we should have decent coverage because in CI we run our Jest test suite against the release artifacts.
7cf1e59
to
67bef4b
Compare
Our expectation is that if you're using the UMD builds, you're not loading them through a normal module system like require or import. Instead you're probably copying the files directly or loading them from a CDN like unpkg.
Summary: This sync includes the following changes: - **[27b569969](facebook/react@27b569969 )**: Simplify cache pool contexts ([#23280](facebook/react#23280)) //<Andrew Clark>// - **[1fb0d0687](facebook/react@1fb0d0687 )**: [Devtools][Transition Tracing] Add Transition callbacks to createRoot ([#23276](facebook/react#23276)) //<Luna Ruan>// - **[a6987bee7](facebook/react@a6987bee7 )**: add <TracingMarker> component boilerplate ([#23275](facebook/react#23275)) //<Luna Ruan>// - **[796fff548](facebook/react@796fff548 )**: Allow suspending outside a Suspense boundary ([#23267](facebook/react#23267)) //<Andrew Clark>// - **[64223fed8](facebook/react@64223fed8 )**: Fix: Multiple hydration errors in same render ([#23273](facebook/react#23273)) //<Andrew Clark>// - **[efd8f6442](facebook/react@efd8f6442 )**: Resolve default onRecoverableError at root init ([#23264](facebook/react#23264)) //<Andrew Clark>// - **[e0af1aabe](facebook/react@e0af1aabe )**: Fix wrong context argument to `apply` //<Andrew Clark>// - **[9b5e0517b](facebook/react@9b5e0517b )**: Remove deprecated wildcard folder mapping ([#23256](facebook/react#23256)) //<Andrew Clark>// - **[274b9fb16](facebook/react@274b9fb16 )**: Remove path resolution from internal forks plugin ([#23255](facebook/react#23255)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions a3bde79...27b5699 jest_e2e[run_all_tests] Reviewed By: rickhanlonii, kacieb Differential Revision: D34241986 fbshipit-source-id: f6ab62df2a918728864283b4f13201275eb3b8a3
* Remove deprecated folder mapping Node v16 deprecated the use of trailing "/" to define subpath folder mappings in the "exports" field of package.json. The recommendation is to explicitly list all our exports. We already do that for all our public modules. I believe the only reason we have a wildcard pattern is because our package.json files are also used at build time (by Rollup) to resolve internal source modules that don't appear in the final npm artifact. Changing trailing "/" to "/*" fixes the warnings. See https://nodejs.org/api/packages.html#subpath-patterns for more info. Since the wildcard pattern only exists so our build script has access to internal at build time, I've scoped the wildcard to "/src/*". Because our public modules are located outside the "src" directory, this means deep imports of our modules will no longer work: only packages that are listed in the "exports" field. The only two affected packages are react-dom and react. We need to be sure that all our public modules are still reachable. I audited the exports by comparing the entries to the "files" field in package.json, which represents a complete list of the files that are included in the final release artifact. At some point, we should add an e2e packaging test to prevent regressions; for now, we should have decent coverage because in CI we run our Jest test suite against the release artifacts. * Remove umd from exports Our expectation is that if you're using the UMD builds, you're not loading them through a normal module system like require or import. Instead you're probably copying the files directly or loading them from a CDN like unpkg.
Summary: This sync includes the following changes: - **[27b569969](facebook/react@27b569969 )**: Simplify cache pool contexts ([facebook#23280](facebook/react#23280)) //<Andrew Clark>// - **[1fb0d0687](facebook/react@1fb0d0687 )**: [Devtools][Transition Tracing] Add Transition callbacks to createRoot ([facebook#23276](facebook/react#23276)) //<Luna Ruan>// - **[a6987bee7](facebook/react@a6987bee7 )**: add <TracingMarker> component boilerplate ([facebook#23275](facebook/react#23275)) //<Luna Ruan>// - **[796fff548](facebook/react@796fff548 )**: Allow suspending outside a Suspense boundary ([facebook#23267](facebook/react#23267)) //<Andrew Clark>// - **[64223fed8](facebook/react@64223fed8 )**: Fix: Multiple hydration errors in same render ([facebook#23273](facebook/react#23273)) //<Andrew Clark>// - **[efd8f6442](facebook/react@efd8f6442 )**: Resolve default onRecoverableError at root init ([facebook#23264](facebook/react#23264)) //<Andrew Clark>// - **[e0af1aabe](facebook/react@e0af1aabe )**: Fix wrong context argument to `apply` //<Andrew Clark>// - **[9b5e0517b](facebook/react@9b5e0517b )**: Remove deprecated wildcard folder mapping ([facebook#23256](facebook/react#23256)) //<Andrew Clark>// - **[274b9fb16](facebook/react@274b9fb16 )**: Remove path resolution from internal forks plugin ([facebook#23255](facebook/react#23255)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions a3bde79...27b5699 jest_e2e[run_all_tests] Reviewed By: rickhanlonii, kacieb Differential Revision: D34241986 fbshipit-source-id: f6ab62df2a918728864283b4f13201275eb3b8a3
Node v16 deprecated the use of trailing "/" to define subpath folder mappings in the "exports" field of package.json.
The recommendation is to explicitly list all our exports. We already do that for all our public modules. I believe the only reason we have a wildcard pattern is because our package.json files are also used at build time (by Rollup) to resolve internal source modules that don't appear in the final npm artifact.
Changing trailing "/" to "/*" fixes the warnings. See https://nodejs.org/api/packages.html#subpath-patterns for more info.
Since the wildcard pattern only exists so our build script has access to internal at build time, I've scoped the wildcard to "/src/*". Because our public modules are located outside the "src" directory, this means deep imports of our modules will no longer work: only packages that are listed in the "exports" field.
The only two affected packages are react-dom and react. We need to be sure that all our public modules are still reachable. I audited the exports by comparing the entries to the "files" field in package.json, which represents a complete list of the files that are included in the final release artifact.
At some point, we should add an e2e packaging test to prevent regressions; for now, we should have decent coverage because in CI we run our Jest test suite against the release artifacts.