Investigate uncovered differences between development and production #11618

gaearon · 2017-11-22T02:31:49Z

Follow up from #11616.

Here's one I found. These two invariants:

react/packages/react-reconciler/src/ReactFiberClassComponent.js

Lines 330 to 343 in 7e71273

    
           if (state && (typeof state !== 'object' || isArray(state))) { 
        
             invariant( 
        
               false, 
        
               '%s.state: must be set to an object or null', 
        
               getComponentName(workInProgress), 
        
             ); 
        
           } 
        
           if (typeof instance.getChildContext === 'function') { 
        
             invariant( 
        
               typeof workInProgress.type.childContextTypes === 'object', 
        
               '%s.getChildContext(): childContextTypes must be defined in order to ' + 
        
                 'use getChildContext().', 
        
               getComponentName(workInProgress), 
        
             );

are in a function that's behind a DEV block:

react/packages/react-reconciler/src/ReactFiberClassComponent.js

Lines 447 to 449 in 7e71273

    
           if (__DEV__) { 
        
             checkClassInstance(workInProgress); 
        
           }

I don't think this is intentional, but need to verify.

There may be more. I'll use this issue to track while #11616 preserves existing behavior.

It is very likely unintentional but I don't want to change behavior in this PR. Filed a follow up as facebook#11618.

gaearon · 2017-11-22T11:32:32Z

Here’s another difference: in development, putting a string ref on a functional component throws with:

Stateless function components cannot have refs.

but in production gives you:

Element ref was specified as a string but no owner was set.

* Move Jest setup files to /dev/ subdirectory * Clone Jest /dev/ files into /prod/ * Move shared code into scripts/jest * Move Jest config into the scripts folder * Fix the equivalence test It fails because the config is now passed to Jest explicitly. But the test doesn't know about the config. To fix this, we just run it via `yarn test` (which includes the config). We already depend on Yarn for development anyway. * Add yarn test-prod to run Jest with production environment * Actually flip the production tests to run in prod environment This produces a bunch of errors: Test Suites: 64 failed, 58 passed, 122 total Tests: 740 failed, 26 skipped, 1809 passed, 2575 total Snapshots: 16 failed, 4 passed, 20 total * Ignore expectDev() calls in production Down from 740 to 175 failed. Test Suites: 44 failed, 78 passed, 122 total Tests: 175 failed, 26 skipped, 2374 passed, 2575 total Snapshots: 16 failed, 4 passed, 20 total * Decode errors so tests can assert on their messages Down from 175 to 129. Test Suites: 33 failed, 89 passed, 122 total Tests: 129 failed, 1029 skipped, 1417 passed, 2575 total Snapshots: 16 failed, 4 passed, 20 total * Remove ReactDOMProduction-test There is no need for it now. The only test that was special is moved into ReactDOM-test. * Remove production switches from ReactErrorUtils The tests now run in production in a separate pass. * Add and use spyOnDev() for warnings This ensures that by default we expect no warnings in production bundles. If the warning *is* expected, use the regular spyOn() method. This currently breaks all expectDev() assertions without __DEV__ blocks so we go back to: Test Suites: 56 failed, 65 passed, 121 total Tests: 379 failed, 1029 skipped, 1148 passed, 2556 total Snapshots: 16 failed, 4 passed, 20 total * Replace expectDev() with expect() in __DEV__ blocks We started using spyOnDev() for console warnings to ensure we don't *expect* them to occur in production. As a consequence, expectDev() assertions on console.error.calls fail because console.error.calls doesn't exist. This is actually good because it would help catch accidental warnings in production. To solve this, we are getting rid of expectDev() altogether, and instead introduce explicit expectation branches. We'd need them anyway for testing intentional behavior differences. This commit replaces all expectDev() calls with expect() calls in __DEV__ blocks. It also removes a few unnecessary expect() checks that no warnings were produced (by also removing the corresponding spyOnDev() calls). Some DEV-only assertions used plain expect(). Those were also moved into __DEV__ blocks. ReactFiberErrorLogger was special because it console.error()'s in production too. So in that case I intentionally used spyOn() instead of spyOnDev(), and added extra assertions. This gets us down to: Test Suites: 21 failed, 100 passed, 121 total Tests: 72 failed, 26 skipped, 2458 passed, 2556 total Snapshots: 16 failed, 4 passed, 20 total * Enable User Timing API for production testing We could've disabled it, but seems like a good idea to test since we use it at FB. * Test for explicit Object.freeze() differences between PROD and DEV This is one of the few places where DEV and PROD behavior differs for performance reasons. Now we explicitly test both branches. * Run Jest via "yarn test" on CI * Remove unused variable * Assert different error messages * Fix error handling tests This logic is really complicated because of the global ReactFiberErrorLogger mock. I understand it now, so I added TODOs for later. It can be much simpler if we change the rest of the tests that assert uncaught errors to also assert they are logged as warnings. Which mirrors what happens in practice anyway. * Fix more assertions * Change tests to document the DEV/PROD difference for state invariant It is very likely unintentional but I don't want to change behavior in this PR. Filed a follow up as #11618. * Remove unnecessary split between DEV/PROD ref tests * Fix more test message assertions * Make validateDOMNesting tests DEV-only * Fix error message assertions * Document existing DEV/PROD message difference (possible bug) * Change mocking assertions to be DEV-only * Fix the error code test * Fix more error message assertions * Fix the last failing test due to known issue * Run production tests on CI * Unify configuration * Fix coverage script * Remove expectDev from eslintrc * Run everything in band We used to before, too. I just forgot to add the arguments after deleting the script.

Avoids firing warnings multiple times

gaearon · 2017-11-22T18:10:36Z

#11630

* Move Jest setup files to /dev/ subdirectory * Clone Jest /dev/ files into /prod/ * Move shared code into scripts/jest * Move Jest config into the scripts folder * Fix the equivalence test It fails because the config is now passed to Jest explicitly. But the test doesn't know about the config. To fix this, we just run it via `yarn test` (which includes the config). We already depend on Yarn for development anyway. * Add yarn test-prod to run Jest with production environment * Actually flip the production tests to run in prod environment This produces a bunch of errors: Test Suites: 64 failed, 58 passed, 122 total Tests: 740 failed, 26 skipped, 1809 passed, 2575 total Snapshots: 16 failed, 4 passed, 20 total * Ignore expectDev() calls in production Down from 740 to 175 failed. Test Suites: 44 failed, 78 passed, 122 total Tests: 175 failed, 26 skipped, 2374 passed, 2575 total Snapshots: 16 failed, 4 passed, 20 total * Decode errors so tests can assert on their messages Down from 175 to 129. Test Suites: 33 failed, 89 passed, 122 total Tests: 129 failed, 1029 skipped, 1417 passed, 2575 total Snapshots: 16 failed, 4 passed, 20 total * Remove ReactDOMProduction-test There is no need for it now. The only test that was special is moved into ReactDOM-test. * Remove production switches from ReactErrorUtils The tests now run in production in a separate pass. * Add and use spyOnDev() for warnings This ensures that by default we expect no warnings in production bundles. If the warning *is* expected, use the regular spyOn() method. This currently breaks all expectDev() assertions without __DEV__ blocks so we go back to: Test Suites: 56 failed, 65 passed, 121 total Tests: 379 failed, 1029 skipped, 1148 passed, 2556 total Snapshots: 16 failed, 4 passed, 20 total * Replace expectDev() with expect() in __DEV__ blocks We started using spyOnDev() for console warnings to ensure we don't *expect* them to occur in production. As a consequence, expectDev() assertions on console.error.calls fail because console.error.calls doesn't exist. This is actually good because it would help catch accidental warnings in production. To solve this, we are getting rid of expectDev() altogether, and instead introduce explicit expectation branches. We'd need them anyway for testing intentional behavior differences. This commit replaces all expectDev() calls with expect() calls in __DEV__ blocks. It also removes a few unnecessary expect() checks that no warnings were produced (by also removing the corresponding spyOnDev() calls). Some DEV-only assertions used plain expect(). Those were also moved into __DEV__ blocks. ReactFiberErrorLogger was special because it console.error()'s in production too. So in that case I intentionally used spyOn() instead of spyOnDev(), and added extra assertions. This gets us down to: Test Suites: 21 failed, 100 passed, 121 total Tests: 72 failed, 26 skipped, 2458 passed, 2556 total Snapshots: 16 failed, 4 passed, 20 total * Enable User Timing API for production testing We could've disabled it, but seems like a good idea to test since we use it at FB. * Test for explicit Object.freeze() differences between PROD and DEV This is one of the few places where DEV and PROD behavior differs for performance reasons. Now we explicitly test both branches. * Run Jest via "yarn test" on CI * Remove unused variable * Assert different error messages * Fix error handling tests This logic is really complicated because of the global ReactFiberErrorLogger mock. I understand it now, so I added TODOs for later. It can be much simpler if we change the rest of the tests that assert uncaught errors to also assert they are logged as warnings. Which mirrors what happens in practice anyway. * Fix more assertions * Change tests to document the DEV/PROD difference for state invariant It is very likely unintentional but I don't want to change behavior in this PR. Filed a follow up as facebook#11618. * Remove unnecessary split between DEV/PROD ref tests * Fix more test message assertions * Make validateDOMNesting tests DEV-only * Fix error message assertions * Document existing DEV/PROD message difference (possible bug) * Change mocking assertions to be DEV-only * Fix the error code test * Fix more error message assertions * Fix the last failing test due to known issue * Run production tests on CI * Unify configuration * Fix coverage script * Remove expectDev from eslintrc * Run everything in band We used to before, too. I just forgot to add the arguments after deleting the script.

* Move Jest setup files to /dev/ subdirectory * Clone Jest /dev/ files into /prod/ * Move shared code into scripts/jest * Move Jest config into the scripts folder * Fix the equivalence test It fails because the config is now passed to Jest explicitly. But the test doesn't know about the config. To fix this, we just run it via `yarn test` (which includes the config). We already depend on Yarn for development anyway. * Add yarn test-prod to run Jest with production environment * Actually flip the production tests to run in prod environment This produces a bunch of errors: Test Suites: 64 failed, 58 passed, 122 total Tests: 740 failed, 26 skipped, 1809 passed, 2575 total Snapshots: 16 failed, 4 passed, 20 total * Ignore expectDev() calls in production Down from 740 to 175 failed. Test Suites: 44 failed, 78 passed, 122 total Tests: 175 failed, 26 skipped, 2374 passed, 2575 total Snapshots: 16 failed, 4 passed, 20 total * Decode errors so tests can assert on their messages Down from 175 to 129. Test Suites: 33 failed, 89 passed, 122 total Tests: 129 failed, 1029 skipped, 1417 passed, 2575 total Snapshots: 16 failed, 4 passed, 20 total * Remove ReactDOMProduction-test There is no need for it now. The only test that was special is moved into ReactDOM-test. * Remove production switches from ReactErrorUtils The tests now run in production in a separate pass. * Add and use spyOnDev() for warnings This ensures that by default we expect no warnings in production bundles. If the warning *is* expected, use the regular spyOn() method. This currently breaks all expectDev() assertions without __DEV__ blocks so we go back to: Test Suites: 56 failed, 65 passed, 121 total Tests: 379 failed, 1029 skipped, 1148 passed, 2556 total Snapshots: 16 failed, 4 passed, 20 total * Replace expectDev() with expect() in __DEV__ blocks We started using spyOnDev() for console warnings to ensure we don't *expect* them to occur in production. As a consequence, expectDev() assertions on console.error.calls fail because console.error.calls doesn't exist. This is actually good because it would help catch accidental warnings in production. To solve this, we are getting rid of expectDev() altogether, and instead introduce explicit expectation branches. We'd need them anyway for testing intentional behavior differences. This commit replaces all expectDev() calls with expect() calls in __DEV__ blocks. It also removes a few unnecessary expect() checks that no warnings were produced (by also removing the corresponding spyOnDev() calls). Some DEV-only assertions used plain expect(). Those were also moved into __DEV__ blocks. ReactFiberErrorLogger was special because it console.error()'s in production too. So in that case I intentionally used spyOn() instead of spyOnDev(), and added extra assertions. This gets us down to: Test Suites: 21 failed, 100 passed, 121 total Tests: 72 failed, 26 skipped, 2458 passed, 2556 total Snapshots: 16 failed, 4 passed, 20 total * Enable User Timing API for production testing We could've disabled it, but seems like a good idea to test since we use it at FB. * Test for explicit Object.freeze() differences between PROD and DEV This is one of the few places where DEV and PROD behavior differs for performance reasons. Now we explicitly test both branches. * Run Jest via "yarn test" on CI * Remove unused variable * Assert different error messages * Fix error handling tests This logic is really complicated because of the global ReactFiberErrorLogger mock. I understand it now, so I added TODOs for later. It can be much simpler if we change the rest of the tests that assert uncaught errors to also assert they are logged as warnings. Which mirrors what happens in practice anyway. * Fix more assertions * Change tests to document the DEV/PROD difference for state invariant It is very likely unintentional but I don't want to change behavior in this PR. Filed a follow up as facebook/react#11618. * Remove unnecessary split between DEV/PROD ref tests * Fix more test message assertions * Make validateDOMNesting tests DEV-only * Fix error message assertions * Document existing DEV/PROD message difference (possible bug) * Change mocking assertions to be DEV-only * Fix the error code test * Fix more error message assertions * Fix the last failing test due to known issue * Run production tests on CI * Unify configuration * Fix coverage script * Remove expectDev from eslintrc * Run everything in band We used to before, too. I just forgot to add the arguments after deleting the script.

gaearon added a commit to gaearon/react that referenced this issue Nov 22, 2017

Change tests to document the DEV/PROD difference for state invariant

9208e42

It is very likely unintentional but I don't want to change behavior in this PR. Filed a follow up as facebook#11618.

gaearon added the Type: Needs Investigation label Nov 22, 2017

gaearon mentioned this issue Nov 22, 2017

Run Jest in production mode #11616

Merged

gaearon referenced this issue Nov 22, 2017

Only perform class checks on initial mount

4d81744

Avoids firing warnings multiple times

gaearon mentioned this issue Nov 22, 2017

Change DEV-only invariants to be warnings #11630

Merged

gaearon closed this as completed in #11630 Nov 22, 2017

snyk-bot mentioned this issue Mar 17, 2021

[Snyk] Fix for 2 vulnerabilities ryan-ally/react-1#44

Open

This was referenced Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#161

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#162

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#163

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#164

Open

This was referenced Dec 25, 2022

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#187

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#188

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#190

Open

jackromo888 mentioned this issue Dec 26, 2022

[Snyk] Fix for 1 vulnerabilities jackromo888/react#150

Open

ryan-ally mentioned this issue Dec 26, 2022

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#191

Open

ryan-ally mentioned this issue Jan 9, 2023

[Snyk] Security upgrade webpack from 4.46.0 to 5.0.0 ryan-ally/react-1#195

Open

snyk-bot mentioned this issue Jan 9, 2023

[Snyk] Security upgrade webpack from 4.46.0 to 5.0.0 ryan-ally/react-1#196

Open

ryan-ally mentioned this issue Jan 10, 2023

[Snyk] Security upgrade webpack from 4.46.0 to 5.0.0 ryan-ally/react-1#198

Open

jackromo888 mentioned this issue Jan 10, 2023

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 jackromo888/react#171

Open

ryan-ally mentioned this issue Jan 10, 2023

[Snyk] Security upgrade webpack from 4.46.0 to 5.0.0 ryan-ally/react-1#199

Open

ryan-ally mentioned this issue Nov 27, 2023

[Snyk] Fix for 5 vulnerabilities ryan-ally/react-1#250

Open

DevProp mentioned this issue Nov 27, 2023

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 DevProp/react#74

Open

This was referenced Nov 27, 2023

[Snyk] Fix for 3 vulnerabilities ryan-ally/react-1#252

Open

[Snyk] Fix for 3 vulnerabilities ryan-ally/react-1#253

Open

CVSpook mentioned this issue Nov 28, 2023

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 CVSpook/react#60

Open

xotik33 mentioned this issue Nov 28, 2023

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 xotik33/react#29

Open

jackromo888 mentioned this issue Nov 29, 2023

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 jackromo888/react#283

Open

CVSpook mentioned this issue Dec 19, 2023

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 CVSpook/react#76

Open

AlexanderGrovnes mentioned this issue Dec 19, 2023

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 AlexanderGrovnes/react#87

Open

jackromo888 mentioned this issue Dec 20, 2023

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 jackromo888/react#313

Open

DevProp mentioned this issue May 13, 2024

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 DevProp/react#149

Open

xotik33 mentioned this issue May 13, 2024

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 xotik33/react#111

Open

CVSpook mentioned this issue May 13, 2024

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 CVSpook/react#139

Open

AlexanderGrovnes mentioned this issue May 13, 2024

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 AlexanderGrovnes/react#156

Open

This was referenced May 13, 2024

[Snyk] Fix for 2 vulnerabilities ryan-ally/react-1#285

Open

[Snyk] Fix for 2 vulnerabilities ryan-ally/react-1#286

Open

[Snyk] Fix for 2 vulnerabilities ryan-ally/react-1#288

Open

jackromo888 mentioned this issue May 14, 2024

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 jackromo888/react#374

Open

ryan-ally mentioned this issue May 15, 2024

[Snyk] Fix for 2 vulnerabilities ryan-ally/react-1#291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Investigate uncovered differences between development and production #11618

Investigate uncovered differences between development and production #11618

gaearon commented Nov 22, 2017

gaearon commented Nov 22, 2017

gaearon commented Nov 22, 2017

Investigate uncovered differences between development and production #11618

Investigate uncovered differences between development and production #11618

Comments

gaearon commented Nov 22, 2017

gaearon commented Nov 22, 2017

gaearon commented Nov 22, 2017