Skip to content

Revert "Disable Integration job on main branch" #955

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

Closed
wants to merge 1 commit into from

Conversation

Saadnajmi
Copy link
Collaborator

Reverts #947
Closes #948

@Saadnajmi Saadnajmi requested a review from alloy as a code owner January 15, 2022 00:28
@Saadnajmi
Copy link
Collaborator Author

Needs #954 it seems

@Saadnajmi
Copy link
Collaborator Author

Closing in favor of #947 which is off my fork so I can actually edit files

@Saadnajmi Saadnajmi closed this Jan 15, 2022
@Saadnajmi Saadnajmi deleted the revert-947-disable-integration-on-main branch January 15, 2022 02:56
NickGerleman pushed a commit to NickGerleman/react-native that referenced this pull request Mar 24, 2023
Summary:
X-link: facebook/metro#955

Pull Request resolved: facebook#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:
    - facebook@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
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jun 15, 2023
Summary:
X-link: facebook/metro#955

Pull Request resolved: facebook#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:
    - facebook@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Integration test fails on main
1 participant