From f290eef63128ec8932a202a9cdf32ef7ca203ba8 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 8 Nov 2019 11:08:56 -0500 Subject: [PATCH] Simplify delayed watching of lazy modules. (#10763) * Avoid modifying unibuild.watchSet in PackageSourceBatch._watchOutputFiles. Should fix #10736 by preventing old hashes from remaining in unibuild.watchSet, which was sometimes causing isUpToDate to fail immediately upon restart. * Regression test for issue #10736. --- tools/isobuild/bundler.js | 7 ++++ tools/isobuild/compiler-plugin.js | 36 +++++++++++-------- .../tests/apps/client-refresh/imports/both.js | 1 + .../node_modules/some-package/index.js | 1 + tools/tests/client-refresh.js | 15 ++++++++ 5 files changed, 46 insertions(+), 14 deletions(-) create mode 100644 tools/tests/apps/client-refresh/imports/node_modules/some-package/index.js diff --git a/tools/isobuild/bundler.js b/tools/isobuild/bundler.js index 11a1eb6b5c..af7446bf0f 100644 --- a/tools/isobuild/bundler.js +++ b/tools/isobuild/bundler.js @@ -1146,6 +1146,13 @@ class Target { // that were used in these resources. Depend on them as well. // XXX assumes that this merges cleanly this.watchSet.merge(unibuild.pkg.pluginWatchSet); + + const entry = jsOutputFilesMap.get(unibuild.pkg.name || null); + if (entry && entry.importScannerWatchSet) { + // Populated in PackageSourceBatch._watchOutputFiles, based on the + // ImportScanner's knowledge of which modules are really imported. + this.watchSet.merge(entry.importScannerWatchSet); + } }); if (buildmessage.jobHasMessages()) { diff --git a/tools/isobuild/compiler-plugin.js b/tools/isobuild/compiler-plugin.js index b29adce44c..12aa20d1fd 100644 --- a/tools/isobuild/compiler-plugin.js +++ b/tools/isobuild/compiler-plugin.js @@ -9,7 +9,11 @@ var util = require('util'); var _ = require('underscore'); var Profile = require('../tool-env/profile').Profile; import assert from "assert"; -import {sha1, readAndWatchFileWithHash} from '../fs/watch'; +import { + WatchSet, + sha1, + readAndWatchFileWithHash, +} from '../fs/watch'; import LRU from 'lru-cache'; import {sourceMapLength} from '../utils/utils.js'; import {Console} from '../console/console.js'; @@ -1261,6 +1265,7 @@ export class PackageSourceBatch { files: inputFiles, mainModule: _.find(inputFiles, file => file.mainModule) || null, batch, + importScannerWatchSet: new WatchSet(), }); }); @@ -1337,17 +1342,19 @@ export class PackageSourceBatch { } }); + const entry = map.get(name); + const scanner = new ImportScanner({ name, bundleArch: batch.processor.arch, extensions: batch.importExtensions, sourceRoot: batch.sourceRoot, nodeModulesPaths, - watchSet: batch.unibuild.watchSet, + watchSet: entry.importScannerWatchSet, cacheDir: batch.scannerCacheDir, }); - scanner.addInputFiles(map.get(name).files); + scanner.addInputFiles(entry.files); if (batch.useMeteorInstall) { scanner.scanImports(); @@ -1495,20 +1502,21 @@ export class PackageSourceBatch { absPath = sourcePath && files.pathJoin(entry.batch.sourceRoot, sourcePath), } = file; - const watchSet = entry.batch.unibuild.watchSet; + const { importScannerWatchSet } = entry; if ( typeof absPath === "string" && - // Blindly calling watchSet.addFile would be logically correct here, - // but we can save the cost of calling optimisticHashOrNull(absPath) - // if the watchSet already knows about the file and it was not marked - // as potentially unused. - ! watchSet.isDefinitelyUsed(absPath) + // Blindly calling importScannerWatchSet.addFile would be + // logically correct here, but we can save the cost of calling + // optimisticHashOrNull(absPath) if the importScannerWatchSet + // already knows about the file and it has not been marked as + // potentially unused. + ! importScannerWatchSet.isDefinitelyUsed(absPath) ) { - // If this file was previously added to the unibuild.watchSet using - // the addPotentiallyUnusedFile method (see compileUnibuild), calling - // addFile here will update its usage status to reflect that the - // ImportScanner did, in fact, end up "using" the file. - watchSet.addFile(absPath, optimisticHashOrNull(absPath)); + // If this file was previously added to the importScannerWatchSet + // using the addPotentiallyUnusedFile method (see compileUnibuild), + // calling addFile here will update its usage status to reflect that + // the ImportScanner did, in fact, end up "using" the file. + importScannerWatchSet.addFile(absPath, optimisticHashOrNull(absPath)); } }); }); diff --git a/tools/tests/apps/client-refresh/imports/both.js b/tools/tests/apps/client-refresh/imports/both.js index 652a4168ea..8489e56285 100644 --- a/tools/tests/apps/client-refresh/imports/both.js +++ b/tools/tests/apps/client-refresh/imports/both.js @@ -1 +1,2 @@ +import "some-package"; console.log(module.id, 0); diff --git a/tools/tests/apps/client-refresh/imports/node_modules/some-package/index.js b/tools/tests/apps/client-refresh/imports/node_modules/some-package/index.js new file mode 100644 index 0000000000..652a4168ea --- /dev/null +++ b/tools/tests/apps/client-refresh/imports/node_modules/some-package/index.js @@ -0,0 +1 @@ +console.log(module.id, 0); diff --git a/tools/tests/client-refresh.js b/tools/tests/client-refresh.js index e9958141a6..8aab53b096 100644 --- a/tools/tests/client-refresh.js +++ b/tools/tests/client-refresh.js @@ -30,6 +30,21 @@ selftest.define("client refresh for application code", () => testHelper({ }, })); +selftest.define("client refresh for non-npm node_modules", () => testHelper({ + client: { + path: "client/main.js", + id: "/client/main.js", + }, + server: { + path: "server/main.js", + id: "/server/main.js", + }, + both: { + path: "imports/node_modules/some-package/index.js", + id: "/imports/node_modules/some-package/index.js", + }, +})); + function testHelper(pathsAndIds) { const s = new selftest.Sandbox(); s.createApp("myapp", "client-refresh");