Skip to content
This repository has been archived by the owner on Mar 21, 2018. It is now read-only.

Make the additional processes run with stock node #1021

Merged
merged 4 commits into from
Aug 29, 2016
Merged

Make the additional processes run with stock node #1021

merged 4 commits into from
Aug 29, 2016

Conversation

Mossop
Copy link
Member

@Mossop Mossop commented Aug 26, 2016

There are a number of parts to this:

For running and packaging we copy the system node to the lib directory.

We split into two package.json files. The package.json in the app directory
contains only dependencies that can't be webpacked either because of native
code or because webpack isn't clever enough. The top-level package.json
dependencies key contains the other dependencies that are needed by the app code
and can be webpacked. The top-level package.json devDependencies contains any
other dependencies needed by build/test code.

This structure means that app/node_modules contains only the dependencies that
actually ship as-is in the final package. Part of webpack now copies all of that
into the lib directory to be used by the app.

The test test-dependencies replaces test-packages and test-devdeps and does a
thorough verification that other than a few exceptions the above rules are met.

Eslints import plugin isn't smart enough for a two package.json structures so
one of its rules had to be replaced.

All of this means we can get rid of module rebuilding for electron since the
native code now runs in pure node.

@Mossop
Copy link
Member Author

Mossop commented Aug 26, 2016

@bgrins Please just look at the last three commits, the first is identical to what you reviewed previously.

I'm not sure the babel changes are necessary but they seem useful anyway.

CI was failing with file copy errors for node and node_modules, it seems that the copy-webpack-plugin isn't great when used with a lot of files (webpack-contrib/copy-webpack-plugin#59) and their suggested fix with graceful-fs didn't help so I just abandoned webpack for those bits. It means no file watching for that but that doesn't seem like a big problem.

Also added some docs on what the two package.json files are for.

@@ -0,0 +1,19 @@
Tofino uses a two package manifest setup where node modules required to ship as-is with the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me - maybe name the file package-manifests to be consistent with other files in the dir

@bgrins
Copy link
Member

bgrins commented Aug 29, 2016

Looks fine to me

@bgrins bgrins added review+ and removed review? labels Aug 29, 2016
Mossop added 4 commits August 29, 2016 12:37
There are a number of parts to this:

For running and packaging we copy the system node to the lib directory.

We split into two package.json files. The package.json in the app directory
contains only dependencies that can't be webpacked either because of native
code or because webpack isn't clever enough. The top-level package.json
dependencies key contains the other dependencies that are needed by the app code
and can be webpacked. The top-level package.json devDependencies contains any
other dependencies needed by build/test code.

This structure means that app/node_modules contains only the dependencies that
actually ship as-is in the final package. Part of webpack now copies all of that
into the lib directory to be used by the app.

The test test-dependencies replaces test-packages and test-devdeps and does a
thorough verification that other than a few exceptions the above rules are met.

Eslints import plugin isn't smart enough for a two package.json structures so
one of its rules had to be replaced.

All of this means we can get rid of module rebuilding for electron since the
native code now runs in pure node.
@Mossop Mossop merged commit 3d8d631 into mozilla:master Aug 29, 2016
@Mossop Mossop removed the review+ label Aug 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants