Skip to content
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

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Feb 9, 2022

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.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 9, 2022
@acdlite acdlite changed the title Remove deprecated wildcard Remove deprecated wildcard folder mapping Feb 9, 2022
"./package.json": "./package.json",
"./": "./"
"./unstable_testing": "./unstable_testing.js",
"./umd/*": "./umd/*",
Copy link
Collaborator Author

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

Copy link
Collaborator

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?

@acdlite acdlite marked this pull request as draft February 9, 2022 01:02
@acdlite acdlite force-pushed the remove-deprecated-wildcard branch 3 times, most recently from e667998 to 2fb3805 Compare February 9, 2022 02:46
@@ -50,8 +50,10 @@
"./profiling": "./profiling.js",
"./test-utils": "./test-utils.js",
"./unstable_testing": "./unstable_testing.js",
"./package.json": "./package.json",
"./": "./"
"./testing": "./testing.js",
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

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?

@sizebot
Copy link

sizebot commented Feb 9, 2022

Comparing: 274b9fb...c281dd9

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 130.35 kB 130.35 kB = 41.82 kB 41.82 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 135.53 kB 135.53 kB = 43.35 kB 43.35 kB
facebook-www/ReactDOM-prod.classic.js = 431.14 kB 431.14 kB = 79.11 kB 79.11 kB
facebook-www/ReactDOM-prod.modern.js = 421.08 kB 421.08 kB = 77.68 kB 77.68 kB
facebook-www/ReactDOMForked-prod.classic.js = 431.14 kB 431.14 kB = 79.11 kB 79.11 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against c281dd9

@acdlite acdlite force-pushed the remove-deprecated-wildcard branch from 2fb3805 to 05dd99b Compare February 9, 2022 02:52
@acdlite acdlite marked this pull request as ready for review February 9, 2022 03:02
@acdlite acdlite requested a review from sebmarkbage February 9, 2022 03:02
@acdlite acdlite force-pushed the remove-deprecated-wildcard branch 3 times, most recently from 4db18bc to ee66a87 Compare February 9, 2022 04:41
@@ -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"
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that sounds good

Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

@sebmarkbage sebmarkbage left a 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?

@acdlite acdlite force-pushed the remove-deprecated-wildcard branch from ee66a87 to 7cf1e59 Compare February 9, 2022 16:45
@acdlite
Copy link
Collaborator Author

acdlite commented Feb 9, 2022

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?

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.
@acdlite acdlite force-pushed the remove-deprecated-wildcard branch from 7cf1e59 to 67bef4b Compare February 9, 2022 17:03
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.
@acdlite acdlite merged commit 9b5e051 into facebook:main Feb 9, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 17, 2022
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
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* 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.
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants