Remove 'import' from default Package Exports conditions (facebook/react-native#36584)#955
Closed
huntie wants to merge 1 commit into
Closed
Remove 'import' from default Package Exports conditions (facebook/react-native#36584)#955huntie wants to merge 1 commit into
huntie wants to merge 1 commit into
Conversation
facebook-github-bot
added
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D44303559 |
huntie
changed the title
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D44303559 |
huntie
added a commit
to huntie/metro
that referenced
this pull request
Mar 23, 2023
Summary: Pull Request resolved: react#955 X-link: react/react-native#36584 Changelog: [General][Changed] - Default condition set for experimental Package Exports is now `['require', 'react-native']` The [Exports RFC](https://github.com/react-native-community/discussions-and-proposals/blob/main/proposals/0534-metro-package-exports-support.md) had assumed that supporting the `"import"` condition was a syntax-only difference, given we are not in a Node.js environment — and so was worthwhile to support for maximal ecosystem compatibility. {F915841105} This assumption is similar to [`--moduleResolution bundler` in TypeScript 5.0](microsoft/TypeScript#51669): > bundlers and runtimes that include a range of Node-like resolution features and ESM syntax, but do not enforce the strict resolution rules that accompany ES modules in Node or in the browser > -- microsoft/TypeScript#51669 (comment) However, robhogan has rightly pointed out that **we should not do this!** - ESM (once transpiled) is **not** simply a stricter subset of in-scope features supported by CJS. For example, it supports top-level async, which would be breaking at runtime. - We recently made the same change for our Jest environment: - react/react-native@681d7f8 As such, we are erring on the side of correctness and supporting only `['require', 'react-native']` in our defaults. At runtime, all code run by React Native is anticipated to be CommonJS. `"exports"` will instead allow React Native to correctly select the CommonJS versions of modules from all npm packages. Metro changelog: [Experimental] Package Exports `unstable_conditionNames` now defaults to `['require']` Differential Revision: D44303559 fbshipit-source-id: fd32e5664679fa6d8fe8c6dca8c3d4b43d867589
huntie
force-pushed
the
export-D44303559
branch
from
9def50f to
858d9d5
Compare
Summary: Pull Request resolved: react#955 X-link: react/react-native#36584 Changelog: [General][Changed] - Default condition set for experimental Package Exports is now `['require', 'react-native']` The [Exports RFC](https://github.com/react-native-community/discussions-and-proposals/blob/main/proposals/0534-metro-package-exports-support.md) had assumed that supporting the `"import"` condition was a syntax-only difference, given we are not in a Node.js environment — and so was worthwhile to support for maximal ecosystem compatibility. {F915841105} This assumption is similar to [`--moduleResolution bundler` in TypeScript 5.0](microsoft/TypeScript#51669): > bundlers and runtimes that include a range of Node-like resolution features and ESM syntax, but do not enforce the strict resolution rules that accompany ES modules in Node or in the browser > -- microsoft/TypeScript#51669 (comment) However, robhogan has rightly pointed out that **we should not do this!** - ESM (once transpiled) is **not** simply a stricter subset of in-scope features supported by CJS. For example, it supports top-level async, which would be breaking at runtime. - We recently made the same change for our Jest environment: - react/react-native@681d7f8 As such, we are erring on the side of correctness and supporting only `['require', 'react-native']` in our defaults. At runtime, all code run by React Native is anticipated to be CommonJS. `"exports"` will instead allow React Native to correctly select the CommonJS versions of modules from all npm packages. Metro changelog: [Experimental] Package Exports `unstable_conditionNames` now defaults to `['require']` Reviewed By: robhogan Differential Revision: D44303559 fbshipit-source-id: 0d7395ad0a44019ef4b2cad0b43d78dcecc5ecd0
huntie
force-pushed
the
export-D44303559
branch
from
858d9d5 to
d558e4a
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D44303559 |
Contributor
|
This pull request has been merged in 0b2e7a7. |
facebook-github-bot
pushed a commit
to react/react-native
that referenced
this pull request
Mar 23, 2023
Summary: X-link: react/metro#955 Pull Request resolved: #36584 Changelog: [General][Changed] - Default condition set for experimental Package Exports is now `['require', 'react-native']` The [Exports RFC](https://github.com/react-native-community/discussions-and-proposals/blob/main/proposals/0534-metro-package-exports-support.md) had assumed that supporting the `"import"` condition was a syntax-only difference, given we are not in a Node.js environment — and so was worthwhile to support for maximal ecosystem compatibility. {F915841105} This assumption is similar to [`--moduleResolution bundler` in TypeScript 5.0](microsoft/TypeScript#51669): > bundlers and runtimes that include a range of Node-like resolution features and ESM syntax, but do not enforce the strict resolution rules that accompany ES modules in Node or in the browser > -- microsoft/TypeScript#51669 (comment) However, robhogan has rightly pointed out that **we should not do this!** - ESM (once transpiled) is **not** simply a stricter subset of in-scope features supported by CJS. For example, it supports top-level async, which would be breaking at runtime. - We recently made the same change for our Jest environment: - 681d7f8 As such, we are erring on the side of correctness and supporting only `['require', 'react-native']` in our defaults. At runtime, all code run by React Native is anticipated to be CommonJS. `"exports"` will instead allow React Native to correctly select the CommonJS versions of modules from all npm packages. Metro changelog: [Experimental] Package Exports `unstable_conditionNames` now defaults to `['require']` Reviewed By: robhogan Differential Revision: D44303559 fbshipit-source-id: 0077e547e7775e53d1e4e9c3a9d01347f4fb7d4a
cipolleschi
pushed a commit
to react/react-native
that referenced
this pull request
Mar 31, 2023
Summary: X-link: react/metro#955 Pull Request resolved: #36584 Changelog: [General][Changed] - Default condition set for experimental Package Exports is now `['require', 'react-native']` The [Exports RFC](https://github.com/react-native-community/discussions-and-proposals/blob/main/proposals/0534-metro-package-exports-support.md) had assumed that supporting the `"import"` condition was a syntax-only difference, given we are not in a Node.js environment — and so was worthwhile to support for maximal ecosystem compatibility. {F915841105} This assumption is similar to [`--moduleResolution bundler` in TypeScript 5.0](microsoft/TypeScript#51669): > bundlers and runtimes that include a range of Node-like resolution features and ESM syntax, but do not enforce the strict resolution rules that accompany ES modules in Node or in the browser > -- microsoft/TypeScript#51669 (comment) However, robhogan has rightly pointed out that **we should not do this!** - ESM (once transpiled) is **not** simply a stricter subset of in-scope features supported by CJS. For example, it supports top-level async, which would be breaking at runtime. - We recently made the same change for our Jest environment: - 681d7f8 As such, we are erring on the side of correctness and supporting only `['require', 'react-native']` in our defaults. At runtime, all code run by React Native is anticipated to be CommonJS. `"exports"` will instead allow React Native to correctly select the CommonJS versions of modules from all npm packages. Metro changelog: [Experimental] Package Exports `unstable_conditionNames` now defaults to `['require']` Reviewed By: robhogan Differential Revision: D44303559 fbshipit-source-id: 0077e547e7775e53d1e4e9c3a9d01347f4fb7d4a
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
X-link: react/react-native#36584
The Exports RFC had assumed that supporting the
"import"condition was a syntax-only difference, given we are not in a Node.js environment — and so was worthwhile to support for maximal ecosystem compatibility.This assumption is similar to
--moduleResolution bundlerin TypeScript 5.0:However, robhogan has rightly pointed out that we should not do this!
As such, we are erring on the side of correctness and supporting only
['require', 'react-native']in our defaults. At runtime, all code run by React Native is anticipated to be CommonJS."exports"will instead allow React Native to correctly select the CommonJS versions of modules from all npm packages.'import'based on how the module isimported/require()d. For now this is not in scope.Changelog:
[General][Changed] - Default condition set for experimental Package Exports is now
['require', 'react-native']Metro changelog: [Experimental] Package Exports
unstable_conditionNamesnow defaults to['require']Differential Revision: D44303559