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

Fix misapplying prod error opt-out #24688

Merged
merged 1 commit into from
Jun 8, 2022
Merged

Fix misapplying prod error opt-out #24688

merged 1 commit into from
Jun 8, 2022

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Jun 8, 2022

The eslint-disable-next-line opt out for prod error minification was not properly working. In the build a replacable error was output even though it was not failing the build. This change refactors the code to avoid the erroneous behavior but a fix for the lint may be better.

Before this change there was an new Error(message) that got picked up as FIXME (minify-errors-in-prod): Unminified error message in production build! <expected-error-format>"%s"</expected-error-format>

Strangely, CI did not pick this up as a problem in check_error_codes...

The eslint-disable-next-line opt out for prod error minification was not properly working. In the build a replacable error was output even though it was not failing the build. This change refactors the code to avoid the erroneous behavior but a fix for the lint may be better
@sizebot
Copy link

sizebot commented Jun 8, 2022

Comparing: b345523...039546d

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 = 131.89 kB 131.76 kB = 42.36 kB 42.30 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 137.16 kB 137.03 kB = 43.95 kB 43.89 kB
facebook-www/ReactDOM-prod.classic.js = 441.21 kB 441.08 kB = 80.73 kB 80.67 kB
facebook-www/ReactDOM-prod.modern.js = 426.52 kB 426.39 kB = 78.53 kB 78.47 kB
facebook-www/ReactDOMForked-prod.classic.js = 441.92 kB 441.79 kB = 80.95 kB 80.89 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 039546d

@acdlite
Copy link
Collaborator

acdlite commented Jun 8, 2022

Strangely, CI did not pick this up as a problem in check_error_codes...

Can you figure out why? Would be nice to fix that so this doesn't sneak past again

acdlite added a commit to acdlite/react that referenced this pull request Jun 8, 2022
The diff command was missing a --quiet argument, causing the job not to
fail when unminified messages were found.

This commit will cause CI to fail because there's an unminified error
in main. The unminified error will be fixed by facebook#24688.
@acdlite
Copy link
Collaborator

acdlite commented Jun 8, 2022

#24692 fixes the check_error_codes job so that it fails before your PR

@gnoff
Copy link
Collaborator Author

gnoff commented Jun 8, 2022

Can you figure out why? Would be nice to fix that so this doesn't sneak past again

Seems like you got to it before me. What I don't understand about your fix is how it ever failed? I've definitely had failures from that job before so it can't be broken all the time but the --quiet thing suggests it should have never worked

@gnoff gnoff merged commit 060505e into facebook:main Jun 8, 2022
@gnoff gnoff deleted the error-nit branch June 8, 2022 16:49
@gnoff
Copy link
Collaborator Author

gnoff commented Jun 8, 2022

Also, I'm not terribly familiar with ast walking but it surprises me that the disable next line thing does not work in the position prior to this change. Maybe I'll go learn about that and see if I can get it to actually work as intuition would indicate

@acdlite
Copy link
Collaborator

acdlite commented Jun 8, 2022

There's an ESLint rule that checks the source files. The check_error_codes job is different: it checks the production build artifacts, as a final failsafe.

@gnoff
Copy link
Collaborator Author

gnoff commented Jun 8, 2022

Oh so it works in eslint but not the custom build job

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jun 15, 2022
Summary:
This sync includes the following changes:
- **[5cc2487e0](facebook/react@5cc2487e0 )**: bump versions for next release ([#24725](facebook/react#24725)) //<Josh Story>//
- **[54f17e490](facebook/react@54f17e490 )**: [Transition Tracing] Fix Cache and Transitions Pop Order ([#24719](facebook/react#24719)) //<Luna Ruan>//
- **[7cf8dfd94](facebook/react@7cf8dfd94 )**: [Transition Tracing] Create/Process Marker Complete Callback ([#24700](facebook/react#24700)) //<Luna Ruan>//
- **[327e4a1f9](facebook/react@327e4a1f9 )**: [Follow-up] Land enableClientRenderFallbackOnTextMismatch //<Andrew Clark>//
- **[a8c9cb18b](facebook/react@a8c9cb18b )**: Land enableSuspenseLayoutEffectSemantics flag ([#24713](facebook/react#24713)) //<Andrew Clark>//
- **[a8555c308](facebook/react@a8555c308 )**: [Transition Tracing] Add Tracing Marker Stack ([#24661](facebook/react#24661)) //<Luna Ruan>//
- **[8186b1937](facebook/react@8186b1937 )**: Check for infinite update loops even if unmounted ([#24697](facebook/react#24697)) //<Andrew Clark>//
- **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([#24688](facebook/react#24688)) //<Josh Story>//
- **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([#24689](facebook/react#24689)) //<Mathieu Dutour>//
- **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([#24680](facebook/react#24680)) //<Josh Story>//
- **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([#24685](facebook/react#24685)) //<Andrew Clark>//
- **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([#24683](facebook/react#24683)) //<Sebastian Markbåge>//
- **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([#24591](facebook/react#24591)) //<Josh Story>//
- **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([#24663](facebook/react#24663)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions d300ceb...256aefb

jest_e2e[run_all_tests]

Reviewed By: cortinico

Differential Revision: D37155957

fbshipit-source-id: 4c0afc95abe8fa13c3803584922c8dc0059ff562
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Aug 8, 2022
Summary:
Sync goes to v18.2 release.

I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days.

This sync includes the following changes:
- **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([#24688](facebook/react#24688)) //<Josh Story>//
- **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([#24689](facebook/react#24689)) //<Mathieu Dutour>//
- **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([#24680](facebook/react#24680)) //<Josh Story>//
- **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([#24685](facebook/react#24685)) //<Andrew Clark>//
- **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([#24683](facebook/react#24683)) //<Sebastian Markbåge>//
- **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([#24591](facebook/react#24591)) //<Josh Story>//
- **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([#24663](facebook/react#24663)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions d300ceb...9e3b772

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross

Differential Revision: D38496392

fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
Summary:
Sync goes to v18.2 release.

I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days.

This sync includes the following changes:
- **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([facebook#24688](facebook/react#24688)) //<Josh Story>//
- **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([facebook#24689](facebook/react#24689)) //<Mathieu Dutour>//
- **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([facebook#24680](facebook/react#24680)) //<Josh Story>//
- **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([facebook#24685](facebook/react#24685)) //<Andrew Clark>//
- **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([facebook#24683](facebook/react#24683)) //<Sebastian Markbåge>//
- **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([facebook#24591](facebook/react#24591)) //<Josh Story>//
- **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([facebook#24663](facebook/react#24663)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions d300ceb...9e3b772

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross

Differential Revision: D38496392

fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
Summary:
Sync goes to v18.2 release.

I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days.

This sync includes the following changes:
- **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([facebook#24688](facebook/react#24688)) //<Josh Story>//
- **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([facebook#24689](facebook/react#24689)) //<Mathieu Dutour>//
- **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([facebook#24680](facebook/react#24680)) //<Josh Story>//
- **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([facebook#24685](facebook/react#24685)) //<Andrew Clark>//
- **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([facebook#24683](facebook/react#24683)) //<Sebastian Markbåge>//
- **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([facebook#24591](facebook/react#24591)) //<Josh Story>//
- **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([facebook#24663](facebook/react#24663)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions d300ceb...9e3b772

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross

Differential Revision: D38496392

fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants