Fixed broken build script --unsafe-partial flag #22324

bvaughn · 2021-09-15T15:51:56Z

Resolves #22323

The following example command sequence will fail:

# Grab the latest build artifacts from CI
scripts/release/download-experimental-build.js --commit=main

# Rebuild only the local NODE bundle from the source of react-dom
# Leave all other artifacts untouched
yarn build --unsafe-partial --type=NODE_DEV react-dom/index

# Throws

The above sequence should work but instead fails with one of the two errors below:

Error: ENOENT: no such file or directory

Error: SyntaxError: Unexpected end of JSON input

This leaves the repo in a broken state (tests won't run, DevTools test shell will crash, etc).

These errors are thrown because this function prematurely resolves:

react/scripts/rollup/utils.js

Lines 10 to 24 in 67222f0

    
           function asyncCopyTo(from, to) { 
        
             return asyncMkDirP(path.dirname(to)).then( 
        
               () => 
        
                 new Promise((resolve, reject) => { 
        
                   ncp(from, to, error => { 
        
                     if (error) { 
        
                       // Wrap to have a useful stack trace. 
        
                       reject(new Error(error)); 
        
                       return; 
        
                     } 
        
                     resolve(); 
        
                   }); 
        
                 }) 
        
             ); 
        
           }

Either before the package.json file has been copied, or after it has been copied (but while its contents are still empty).

This causes the subsequent read of package.json to throw:

react/scripts/rollup/packaging.js

Lines 145 to 148 in 67222f0

    
           function filterOutEntrypoints(name) { 
        
             // Remove entry point files that are not built in this configuration. 
        
             let jsonPath = `build/node_modules/${name}/package.json`; 
        
             let packageJSON = JSON.parse(readFileSync(jsonPath));

The error above can be verified by adding logging to the asyncTopyTo method:

function asyncCopyTo(from, to) {
  return asyncMkDirP(path.dirname(to)).then(
    () =>
      new Promise((resolve, reject) => {
        ncp(from, to, error => {
          if (error) {
            // Wrap to have a useful stack trace.
            reject(new Error(error));
            return;
          }
          if (to.includes('package.json')){
            console.log(`asyncCopyTo() -> ncp("${from}", "${to}") -> resolve`);

            // This line will either throw (no file) or print an empty string in many cases:
            console.log(require('fs').readFileSync(to).toString());
          }
          resolve();
        });
      })
  );
}

I'm able to "fix" this issue by introducing a small amount of delay, e.g.

diff --git a/scripts/rollup/packaging.js b/scripts/rollup/packaging.js
index eaf78959a6..5844d9116f 100644
--- a/scripts/rollup/packaging.js
+++ b/scripts/rollup/packaging.js
@@ -196,6 +196,10 @@ async function prepareNpmPackage(name) {
     ),
     asyncCopyTo(`packages/${name}/npm`, `build/node_modules/${name}`),
   ]);
+
+  // Wait for copied files to exist; asyncCopyTo() completes prematurely.
+  await new Promise(resolve => setTimeout(resolve, 100));
+
   filterOutEntrypoints(name);
   const tgzName = (
     await asyncExecuteCommand(`npm pack build/node_modules/${name}`)

But that's pretty hacky and fragile.

I evaluated replacing ncp with some other alternatives:

copyfiles - Bad documentation.
cpx – Similar interface, seems to have the exact same race case problem as ncp
recursively-copy - Worked locally but broke CI (see Rollup build script --unsafe-partial flag is broken #22323).

Seems like this is perhaps related to AvianFlu/ncp#127

With this change, I am able to successfully run the command shown above:

This flag was broken due to a buggy race case in the ncp() command. The fix is amittedly a hack but improves on the existing behavior (of leaving the workspace in a broken state).

gaearon

sure

sizebot · 2021-09-15T15:56:15Z

Comparing: 50263d3...c1a5676

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	=	129.49 kB	129.49 kB	=	41.26 kB	41.26 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	132.31 kB	132.31 kB	=	42.21 kB	42.21 kB
facebook-www/ReactDOM-prod.classic.js	=	410.86 kB	410.86 kB	=	76.01 kB	76.01 kB
facebook-www/ReactDOM-prod.modern.js	=	399.43 kB	399.43 kB	=	74.29 kB	74.29 kB
facebook-www/ReactDOMForked-prod.classic.js	=	410.86 kB	410.86 kB	=	76.01 kB	76.01 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against c1a5676

Summary: This sync includes the following changes: - **[f4ac680c7](facebook/react@f4ac680c7 )**: Fixed broken build script --unsafe-partial flag ([#22324](facebook/react#22324)) //<Brian Vaughn>// - **[67222f044](facebook/react@67222f044 )**: [Experiment] Warn if callback ref returns a function ([#22313](facebook/react#22313)) //<Dan Abramov>// - **[263cfa6ec](facebook/react@263cfa6ec )**: [Experimental] Add useInsertionEffect ([#21913](facebook/react#21913)) //<Ricky>// - **[806aaa2e2](facebook/react@806aaa2e2 )**: [useSES shim] Import prefixed native API ([#22310](facebook/react#22310)) //<Andrew Clark>// - **[fd5e01c2e](facebook/react@fd5e01c2e )**: [useSES/extra] Reuse old selection if possible ([#22307](facebook/react#22307)) //<Andrew Clark>// - **[33226fada](facebook/react@33226fada )**: Check for store mutations before commit ([#22290](facebook/react#22290)) //<Andrew Clark>// - **[86c7ca70a](facebook/react@86c7ca70a )**: Fix link ([#22296](facebook/react#22296)) //<Konstantin Popov>// - **[0fd195f29](facebook/react@0fd195f29 )**: update error message to include useLayoutEffect or useEffect on bad e… ([#22279](facebook/react#22279)) //<salazarm>// - **[8f96c6b2a](facebook/react@8f96c6b2a )**: [Bugfix] Prevent infinite update loop caused by a synchronous update in a passive effect ([#22277](facebook/react#22277)) //<Andrew Clark>// - **[4ce89a58d](facebook/react@4ce89a58d )**: Test bad useEffect return value with noop-renderer ([#22258](facebook/react#22258)) //<Sebastian Silbermann>// - **[a3fde2358](facebook/react@a3fde2358 )**: Detect subscriptions wrapped in startTransition ([#22271](facebook/react#22271)) //<salazarm>// Changelog: [General][Changed] - React Native sync for revisions 95d762e...e8feb11 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D30966369 fbshipit-source-id: 6c88e591005deb1fd93493628ef4695add49186c

This flag was broken due to a buggy race case in the ncp() command. The fix is amittedly a hack but improves on the existing behavior (of leaving the workspace in a broken state).

Fixed broken build script --unsafe-partial flag

c1a5676

This flag was broken due to a buggy race case in the ncp() command. The fix is amittedly a hack but improves on the existing behavior (of leaving the workspace in a broken state).

bvaughn requested review from gaearon, jstejada, rickhanlonii, lunaruan and acdlite September 15, 2021 15:51

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 15, 2021

gaearon approved these changes Sep 15, 2021

View reviewed changes

bvaughn merged commit f4ac680 into facebook:main Sep 15, 2021

bvaughn deleted the fix-build-script-ncp branch September 15, 2021 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed broken build script --unsafe-partial flag #22324

Fixed broken build script --unsafe-partial flag #22324

Uh oh!

bvaughn commented Sep 15, 2021 •

edited

Loading

Uh oh!

gaearon left a comment

Uh oh!

sizebot commented Sep 15, 2021

Uh oh!

Uh oh!

	function asyncCopyTo(from, to) {
	return asyncMkDirP(path.dirname(to)).then(
	() =>
	new Promise((resolve, reject) => {
	ncp(from, to, error => {
	if (error) {
	// Wrap to have a useful stack trace.
	reject(new Error(error));
	return;
	}
	resolve();
	});
	})
	);
	}

	function filterOutEntrypoints(name) {
	// Remove entry point files that are not built in this configuration.
	let jsonPath = `build/node_modules/${name}/package.json`;
	let packageJSON = JSON.parse(readFileSync(jsonPath));

Fixed broken build script --unsafe-partial flag #22324

Fixed broken build script --unsafe-partial flag #22324

Uh oh!

Conversation

bvaughn commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

sizebot commented Sep 15, 2021

Critical size changes

Significant size changes

Uh oh!

Uh oh!

bvaughn commented Sep 15, 2021 •

edited

Loading