Issue #11257 - Apply "files" npm whitelist at build time #11404

yu-tian113 · 2017-10-31T06:37:03Z

Compare files/folders in package/npm to "files" entries in package.json, only copy whitelisted files/folders to build.

gaearon · 2017-10-31T11:04:08Z

I'm thinking that we also probably want to fail the build if there is a .js file in the package root that doesn't have an npm/*.js equivalent.

yu-tian113 · 2017-10-31T14:19:27Z

Thanks for the advice @gaearon, I can do a file check after copyBundleIntoNodePackage resolved, and either return another promise to continue the build or fail it if files don't match.
My only question is I'm not sure in what cases, in npm consumption packages, a .js file can be added to package root from somewhere other than npm/*.js, did I miss anything or is this a safeguard for anything unexpected in the future?

gaearon · 2017-10-31T14:28:52Z

I'm not sure in what cases, in npm consumption packages, a .js file can be added to package root from somewhere other than npm/*.js

We use files like react-dom/server.js for local testing and bundling, but we need corresponding files like react-dom/npm/server.js to point to compiled bundles in the built packages. We want to ensure that we can't add a new entry point like react-dom/my-entry-point.js that would pass the tests without also adding a corresponding react-dom/npm/my-entry-point.js file and adding that file to "files" in the package.json.

Does this make sense?

yu-tian113 · 2017-11-01T03:46:51Z

Awesome @gaearon.
I see the current react-dom build will fail then because the index.fb.js in the package root does not exist in react-dom/npm.
I'll need to create an equivalent in react-dom/npm to pass this file matching check. I'm thinking

'use strict';
module.exports = require('./index');

It won't be exposed in npm packages as it's not whitelisted in package.json.
How does it sound to you?

gaearon · 2017-11-01T11:14:42Z

Hmm. Good point, I haven't though about this.
Let's just make an exception for .fb.js files?

yu-tian113 · 2017-11-02T14:17:12Z

Hi @gaearon, please review the updated code as per our discussion above.
Lint is failing on the master branch right now, I'll follow up once it's fixed.
Cheers.

gaearon · 2017-11-02T21:53:44Z

Can you rebase now?

yu-tian113 · 2017-11-02T23:29:28Z

Done, please review. Thanks @gaearon.

gaearon

Nits.

gaearon · 2017-11-03T18:43:59Z

scripts/rollup/packaging.js

+  const packageJson = resolve(`${from}/package.json`);
+  const whitelistedFiles = fs.existsSync(packageJson)
+    ? require(packageJson).files
+    : null;


Maybe just fall back to []? Then you don't have to check for existence later.

Agree. Also need to check require(packageJson).files is not undefined, as files entry is missing in some package.json(e.g. react-call-return).
Updated to
const whitelistedFiles = (fs.existsSync(packageJson) && require(packageJson).files) || [];
And deleted the existance check below.
~~if (whitelistedFiles && whitelistedFiles.length > 0) {}~~
if (whitelistedFiles.length > 0) {}

gaearon · 2017-11-03T18:44:28Z

scripts/rollup/packaging.js

+    }
+  });
+  if (whitelistedFiles && whitelistedFiles.length > 0) {
+    let asyncCopyProxies = [];


Useless assignment: it's reassigned right afterwards. Let's combine?

Agree, done.

gaearon · 2017-11-03T18:44:46Z

scripts/rollup/packaging.js

+  if (whitelistedFiles && whitelistedFiles.length > 0) {
+    let asyncCopyProxies = [];
+    asyncCopyProxies = npmRootFiles.reduce((proxies, file) => {
+      if (


A comment here would help understand what this does and why

Comments added

gaearon · 2017-11-03T18:45:04Z

scripts/rollup/packaging.js

+      return proxies;
+    }, asyncCopyProxies);
+    return Promise.all([
+      ...asyncCopyProxies,


Maybe promisesForForwardingModules?

hehe, sorry, didn't realise I typed these promises as proxies.
Variables renamed.

yu-tian113 · 2017-11-04T03:55:18Z

Thanks @gaearon. All updated, please see my comments.

gaearon · 2017-11-04T18:13:21Z

Shouldn't we also fail if an entry point is not in files in package.json?

yu-tian113 · 2017-11-06T13:49:40Z

Good point, the problem is some package like react-call-return omits the files field, which indicates

everything except automatically-excluded files will be included in the publish

I'm going to check the existence of files field, if not exists, everything in ./npm are whitelisted and will be copied to build.

Another defect l noticed, wildcard patterns("glob/*.{js,json}") in files field is not supported properly in the current solution.

Working on these two issues now, talk soon.

gaearon · 2017-11-07T14:05:56Z

Good point, the problem is some package like react-call-return omits the files field

This actually sounds like a problem we should fix in that package. 😉
Let's mandate that the field exists.

gaearon · 2017-11-07T14:08:25Z

I fixed react-call-return in b2ff29d. We should throw if files doesn't exist unless it is a private package (in which case it's fine).

yu-tian113 · 2017-11-07T15:33:53Z

No problem, thanks for the fix.
Just done some updates to handle these and also the wildcard pattern issue I mentioned above.
All the checks we added should work with globs like *.js in files now.
Please review.

gaearon · 2017-11-13T15:43:31Z

scripts/rollup/packaging.js

+      `'files' field is missing from package.json in package ${packageName}`
+    );
+    process.exit(1);
+  } else {


Since this is a hard exit, can you remove the else and just leave it after this code unindented?

- Terminate the build if 'files' field is missing from package.json - Terminate the build if any entry point doesn’t have equivalent in ./npm folder - Handle patterns in 'files' field

yu-tian113 · 2017-11-14T00:05:51Z

Yes, makes sense, please review, thanks @gaearon.

gaearon · 2017-11-21T14:49:13Z

Seems like file permissions were changed accidentally?

scripts/rollup/packaging.js 100644 → 100755

gaearon · 2017-11-21T14:55:59Z

scripts/rollup/packaging.js

+  fs.mkdirSync(to);
+  let existingDirCache = {};
+  // looping through entries(single file / directory / pattern) in `files`
+  const whitelistedFiles = whitelist.reduce((list, pattern) => {


This logic is a bit complicated. Is there any module we can use for this? Or maybe there's some simpler way to write it? At least let's extract it to a separate function.

Thanks @gaearon, some modules may help with bits and pieces, but I'm a little hesitant including any as new dev dependencies, considering we already use things like ncp and glob(which these modules are also build upon).
The other reason I did't use a prebuild module is I don't want to encapsulate tasks together. For example, glob composes a whiteListedFiles list during the copy process, which is used later for checking all entry points are whitelisted. If a module doing things like copy with glob, we'll have to run glob again separately to get the list.
node-mkdirp can help creating nested directories. Do you think I can add it as dev dependency?
I will extract it to a separate function, make the code flow more intuitive.

node-mkdirp seems fine. Why didn't we need it before?

…itelist-at-build-time

…with mkdirp.

yu-tian113 · 2017-11-26T13:13:39Z

Thanks @gaearon, mkdirp creates parent directories on the fly, help us get rid of the reduce inside reduce in the old code :)
I didn't know about this module before you suggested making the code simpler. Thought I could just build my own in about 10 lines, but it did complicate the function a lot.
I also reset the file permission back to 644.
Please review.

gaearon · 2017-11-28T14:44:10Z

This still seems complicated. My biggest issue is we're essentially reimplementing npm's files inclusion logic.

I think it would make more sense if we used npm pack instead of copying the directory. Then we'd literally test what npm publish actually does (but "deploy" to our build folder): http://podefr.tumblr.com/post/30488475488/locally-test-your-npm-modules-without-publishing

What do you think?

yu-tian113 · 2017-11-29T13:49:56Z

Sorry, I'm a little lost here. Do you mind explaining the idea using npm pack?
How will it deal with the file replacement(entry points) and copy(Circle.js... in react-art package) from /npm?
Besides, reimplementing the files inclusion logic also make sure the build will warn and fail if an entry point is not whitelisted.
Thanks!

gaearon · 2017-11-29T14:17:45Z

I was thinking that maybe we need to change the build step to be:

Build to a temporary folder
npm pack it
Unpack into our build/packages folder structure

This way we simulate what would actually happen during npm publish.

Besides, reimplementing the files inclusion logic also make sure the build will warn and fail if an entry point is not whitelisted.

That's true. But I added a suite of tests that run on bundles in build/packages/ folder a week ago. So if something is missing there, those tests would fail. In other words, instead of trying to validate it ourselves, we can just "simulate" a publish and then run the tests on the packages folder.

yu-tian113 · 2017-11-30T13:59:37Z

Build to a temporary folder

Here we'll still copy everything inside /npm directory to the temporary folder as we've been doing.

npm pack it
Unpack into our build/packages folder structure

I think npm pack and unpack is pretty handy to prepare the build packages, but not much of help validating the packages, as they just silently skip any missing files. So all the validation need to be handled in the test suite.

I'm happy to give it a try.

gaearon · 2017-11-30T14:39:21Z

So all the validation need to be handled in the test suite.

Yes, but this is enough because our test suite covers all public API (or at least it should—if not, we should fix the test suite), and >92% tests run on bundles.

yu-tian113 · 2017-12-02T13:04:25Z

Hi @gaearon, I've created a new pull request #11750 for the updates as per our discussion above.
Please review, thanks!

gaearon · 2017-12-05T13:55:22Z

We decided to go with #11750 instead.

facebook-github-bot added the CLA Signed label Oct 31, 2017

yu-tian113 force-pushed the #11257-Apply-files-npm-whitelist-at-build-time branch 3 times, most recently from 52a4d3b to af7f44f Compare November 2, 2017 14:03

yu-tian113 force-pushed the #11257-Apply-files-npm-whitelist-at-build-time branch from af7f44f to 9a0189b Compare November 2, 2017 23:18

yu-tian113 force-pushed the #11257-Apply-files-npm-whitelist-at-build-time branch from e2d1516 to 507357f Compare November 2, 2017 23:48

gaearon reviewed Nov 3, 2017

View reviewed changes

yu-tian113 force-pushed the #11257-Apply-files-npm-whitelist-at-build-time branch 2 times, most recently from b4ddd28 to cf2656b Compare November 4, 2017 02:37

yu-tian113 force-pushed the #11257-Apply-files-npm-whitelist-at-build-time branch from cf2656b to d445df9 Compare November 7, 2017 15:13

gaearon reviewed Nov 13, 2017

View reviewed changes

yu-tian113 force-pushed the #11257-Apply-files-npm-whitelist-at-build-time branch 3 times, most recently from 879231a to 8b33c91 Compare November 13, 2017 23:51

- Terminate the build if entry point not whitelisted

703fb32

- Terminate the build if 'files' field is missing from package.json - Terminate the build if any entry point doesn’t have equivalent in ./npm folder - Handle patterns in 'files' field

yu-tian113 force-pushed the #11257-Apply-files-npm-whitelist-at-build-time branch from 9a988d9 to 703fb32 Compare November 13, 2017 23:55

gaearon reviewed Nov 21, 2017

View reviewed changes

yu-tian113 added 3 commits November 26, 2017 17:04

Merge branch 'upstream/master' into facebook#11257-Apply-files-npm-wh…

1781fbc

…itelist-at-build-time

add mkdirp module as dev dependency, replace directory creation code …

64be529

…with mkdirp.

reset file permission to 644

c2417eb

yu-tian113 mentioned this pull request Dec 2, 2017

Issue #11257(Updated) - Change build process to include npm pack and unpacking #11750

Merged

gaearon closed this Dec 5, 2017

Issue #11257 - Apply "files" npm whitelist at build time #11404

Issue #11257 - Apply "files" npm whitelist at build time #11404

Conversation

yu-tian113 commented Oct 31, 2017

gaearon commented Oct 31, 2017

yu-tian113 commented Oct 31, 2017

gaearon commented Oct 31, 2017 • edited Loading

yu-tian113 commented Nov 1, 2017

gaearon commented Nov 1, 2017

yu-tian113 commented Nov 2, 2017

gaearon commented Nov 2, 2017

yu-tian113 commented Nov 2, 2017

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yu-tian113 commented Nov 4, 2017

gaearon commented Nov 4, 2017

yu-tian113 commented Nov 6, 2017

gaearon commented Nov 7, 2017

gaearon commented Nov 7, 2017

yu-tian113 commented Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

yu-tian113 commented Nov 14, 2017

gaearon commented Nov 21, 2017

Choose a reason for hiding this comment

yu-tian113 Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yu-tian113 commented Nov 26, 2017

gaearon commented Nov 28, 2017

yu-tian113 commented Nov 29, 2017

gaearon commented Nov 29, 2017 • edited Loading

yu-tian113 commented Nov 30, 2017

gaearon commented Nov 30, 2017

yu-tian113 commented Dec 2, 2017

gaearon commented Dec 5, 2017

gaearon commented Oct 31, 2017 •

edited

Loading

yu-tian113 commented Nov 7, 2017 •

edited

Loading

yu-tian113 Nov 22, 2017 •

edited

Loading

gaearon commented Nov 29, 2017 •

edited

Loading