Skip to content

Conversation

@TatianaKapos
Copy link
Contributor

@TatianaKapos TatianaKapos commented Oct 31, 2022

facebook/react-native@b0aba1b...8cdc9e7

TatianaKapos and others added 30 commits October 28, 2022 17:55
@TatianaKapos TatianaKapos changed the title [WIP] Integration 9/26/22 Integration 9/26/22 Nov 9, 2022
@TatianaKapos TatianaKapos marked this pull request as ready for review November 9, 2022 19:41
@TatianaKapos TatianaKapos requested review from a team as code owners November 9, 2022 19:41
'<rootDir>/jest/assetFileTransformer.js',
},
transformIgnorePatterns: [],
transformIgnorePatterns: ['jest-runner'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this added by us or by Meta? If us, could we add a note in the PR description on why we needed to add this to get Jest working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added it to get jest working again! I'll add it to the PR description but it seems like its a jest issue where we shouldn't be transpiliing jest-runner in the first place and this is the work around (jestjs/jest#13173)

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't affect users of RNW. (or RNWin32). The jest transformer errors out when running against jest-runner.

This code was actually already different than core. Core uses a whitelist of packages within node_modules to run the transforms on, whereas we are running it on everything by default. My guess is due to the monorepo setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool thanks for the context!


// Register to screenshot on each test failure
global.jasmine.getEnv().addReporter({
// TODO - use a jest reporter to create screenshots
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the API that wasn't working for us following the Jest 29 upgrade?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@@ -33,6 +33,9 @@ describe('visitAllPages', () => {
}

for (const api of apiExamples) {
if (api === 'Transforms')
// disable until either transformExample uses units, or that isn't an error
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious on more context for this comment. Is this from Jest upgrade as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a testpage that crashes in core as of this PR: facebook/react-native#34660

facebook/react-native#35292 will fix the test page

Copy link
Contributor

@chiaramooney chiaramooney left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Because there were so many new issues that came up during this integration, let's just make sure we capture what those bugs were and how we solved them in case similar issues emerge down the line!

@TatianaKapos TatianaKapos merged commit ca86014 into microsoft:main Nov 9, 2022
@TatianaKapos TatianaKapos deleted the tk-integrate-9-26-22 branch November 9, 2022 22:12
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.

4 participants