Issue #11257(Updated) - Change build process to include npm pack and unpacking #11750
Conversation
yu-tian113
changed the title
|
Hi @gaearon, this is the new PR for Issue #11257 and old PR #11404. |
scripts/rollup/build.js
Outdated
| @@ -435,6 +435,8 @@ rimraf('build', async () => { | |||
| try { | |||
| // create a new build directory | |||
| fs.mkdirSync('build'); | |||
| // create the .tmp folder for local npm packing and unpacking | |||
| fs.mkdirSync(path.join('build', '.tmp')); | |||
There was a problem hiding this comment.
We should delete this folder after the build.
There was a problem hiding this comment.
Fixed, together with the swap to os.tmpdir()
scripts/rollup/packaging.js
Outdated
| async function createNodePackage(bundleType, packageName, filename) { | ||
| // the only case where we don't want to copy the package is for FB bundles | ||
| if (bundleType === FB_DEV || bundleType === FB_PROD) { | ||
| return; | ||
| } | ||
| await copyNodePackageTemplate(packageName); | ||
| await copyBundleIntoNodePackage(packageName, filename, bundleType); | ||
| // Packing packages locally, simulate npm publish, | ||
| // Then unpacking generated packages to build directory | ||
| await localPackaging(packageName); |
There was a problem hiding this comment.
Let's call it packForNpmAndUnpack()?
scripts/rollup/packaging.js
Outdated
| @@ -85,7 +87,7 @@ async function createFacebookWWWBuild() { | |||
| } | |||
|
|
|||
| async function copyBundleIntoNodePackage(packageName, filename, bundleType) { | |||
| const packageDirectory = resolve(`./build/packages/${packageName}`); | |||
| const packageDirectory = resolve(`./build/.tmp/${packageName}`); | |||
There was a problem hiding this comment.
What do you think about using a folder in os.tmpdir() here instead? Not sure if it would be better, but IMO it's easier to understand if there are fewer files being moved around inside the build folder.
There was a problem hiding this comment.
Agree, updated to use os.tmpdir(), it makes sense to do temp packing and unpacking in the os's temporary folder.
There may be some rare cases, if a use's permission is misconfigured, system temporary folder is not accessible for writing.
Please review.
There was a problem hiding this comment.
There may be some rare cases, if a use's permission is misconfigured, system temporary folder is not accessible for writing.
Yeah. I think let's risk it and see if it's an issue in practice or not.
…ages to corresponding build directories.
yu-tian113
force-pushed
the
#11257-update-build-step-with-pack-unpack
branch
from
0b1971a
to
a68387c
Compare
| @@ -435,6 +437,9 @@ rimraf('build', async () => { | |||
| try { | |||
| // create a new build directory | |||
| fs.mkdirSync('build'); | |||
| // create the temp directory for local npm packing and unpacking | |||
| // in operating system's default temporary directory | |||
| fs.mkdirSync(npmPackagesTmpDir); | |||
There was a problem hiding this comment.
What if it's not empty, and the previous build terminated abruptly before rimraf got a chance to run?
I think ideally we should create a new directory every time, with random uuids as folder names.
There was a problem hiding this comment.
What do you think about removing both build and temp before building, something like rimraf(`{build, ${npmPackagesTmpDir}}`, async () => {...})
Passing the uuided path to all build functions feels quite cumbersome?
There was a problem hiding this comment.
It feels a bit wrong to me that if you have two checkouts of React, running yarn build in them at the same time might produce conflicts. So I'd rather pass it around.
There was a problem hiding this comment.
Thanks for pointing this out! Please review.
|
This is great, thank you! Would you mind submitting another PR to add rudimentary tests for |
|
Awesome! I'll be on it. Thanks! |
Change build process to include npm pack and unpacking generated packages to corresponding build directories.