Skip to content
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.

Sign up for GitHub

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

Update webpack + pack mode #189

Merged
merged 6 commits into from
Dec 21, 2018
Merged

Update webpack + pack mode #189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e775187
update webpack
sokra Dec 21, 2018
eea3f16
close compiler correctly even in case of errors
sokra Dec 21, 2018
33ddfde
Use faster single file cache mode
sokra Dec 21, 2018
d88af64
update to new externals signature
sokra Dec 21, 2018
6a67ea6
create a cache per input file
sokra Dec 21, 2018
ae23f75
Use released webpack version from npm instead
sokra Dec 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
"typescript": "^3.2.2",
"vue": "^2.5.17",
"vue-server-renderer": "^2.5.17",
"webpack": "github:webpack/webpack#next",
"webpack": "5.0.0-alpha.0",
"when": "^3.7.8"
}
}
23 changes: 16 additions & 7 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const resolve = require("resolve");
const fs = require("graceful-fs");
const crypto = require("crypto");
const { sep } = require("path");
const webpack = require("webpack");
const MemoryFS = require("memory-fs");
Expand All @@ -15,6 +16,14 @@ const nodeBuiltins = new Set([...require("repl")._builtinLibs, "constants", "mod

const SUPPORTED_EXTENSIONS = [".js", ".json", ".node", ".mjs", ".ts", ".tsx"];

const hashOf = name => {
return crypto
.createHash("md4")
.update(name)
.digest("hex")
.slice(0, 10);
}

module.exports = (
entry,
{
Expand Down Expand Up @@ -53,9 +62,8 @@ module.exports = (
cache: cache === false ? undefined : {
type: "filesystem",
cacheDirectory: typeof cache === 'string' ? cache : nccCacheDir,
name: "ncc",
version: require('../package.json').version,
store: "instant"
Copy link
Contributor

Choose a reason for hiding this comment

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

If switching to the pack mode, should we also now ensure that the cache is a per-project cache in the node_modules folder instead of the single global cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would make sense.

Another option is to keep the cache location, but use a different cache.name for each directory.

Like this: name: hashOf(directory)

Copy link
Contributor

Choose a reason for hiding this comment

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

The risk with the global cache with this new storage seems to be disk usage growth right?

I guess this can be mitigated with a simple eviction strategy in due course. But that was another argument for per-project caching.

The argument for global caching is that ideally it would be great if the global cache could be smart enough to know the hashes of the dependencies (yarn.lock again!), and thus share compilations.

Not sure which of the above you want to shoot for here, but will trust your judgement!

Copy link
Member Author

Choose a reason for hiding this comment

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

The per-project cache is garbage-collected, so disk usage should increase only with the number of different entries used on a computer.

name: `ncc_${hashOf(entry)}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be a hash of the project directory, so that if the input changes the cache can still be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've thought about that, but decided against it.

This could be a problem when using ncc for multiple entries in a single directory. They would override the cached generated output files in the cache. This would slow down the rebuilds when switching between these these entries.

This configuration provides consistent fast rebuilds at the cost of high initial build for each entry.

But choosing the other way is also a valid trade-off. I assumed # of rebuilds >> # of initial builds and people may run ncc for multiple entries in parallel.

version: require('../package.json').version
},
optimization: {
nodeEnv: false,
Expand All @@ -78,7 +86,7 @@ module.exports = (
},
// https://github.com/zeit/ncc/pull/29#pullrequestreview-177152175
node: false,
externals: async (context, request, callback) => {
externals: async ({ context, request }, callback) => {
if (externalSet.has(request)) return callback(null, `commonjs ${request}`);
if (request[0] === "." && (request[1] === "/" || request[1] === "." && request[2] === "/")) {
if (request.startsWith("./node_modules/")) request = request.substr(15);
Expand Down Expand Up @@ -208,9 +216,10 @@ module.exports = (
return new Promise((resolve, reject) => {
compiler.run((err, stats) => {
if (err) return reject(err);
if (stats.hasErrors())
return reject(new Error(stats.toString()));
compiler.close(() => {
compiler.close(err => {
if (err) return reject(err);
if (stats.hasErrors())
return reject(new Error(stats.toString()));
resolve();
});
});
Expand Down
7 changes: 4 additions & 3 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11109,9 +11109,10 @@ webpack-sources@^1.1.0, webpack-sources@^1.3.0:
source-list-map "^2.0.0"
source-map "~0.6.1"

"webpack@github:webpack/webpack#next":
version "5.0.0-next"
resolved "https://codeload.github.com/webpack/webpack/tar.gz/3f71a165bf17bfb27138dcba799d9437a9af558c"
[email protected]:
version "5.0.0-alpha.0"
resolved "https://registry.yarnpkg.com/webpack/-/webpack-5.0.0-alpha.0.tgz#6fdd442932ff232da783e629f2ffcf29a571b533"
integrity sha512-dycVcffNaebFKI5Y6yv4yvtzAqr6MahHMBo58I9+YZQUAlf8x7sjVwFbCzxtEKLR3DAihJeQ7RscpqEQqimpuQ==
dependencies:
"@webassemblyjs/ast" "1.7.11"
"@webassemblyjs/helper-module-context" "1.7.11"
Expand Down