-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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
Issue #11257(Updated) - Change build process to include npm pack and unpacking #11750
Issue #11257(Updated) - Change build process to include npm pack and unpacking #11750
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should delete this folder after the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call it packForNpmAndUnpack()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.