-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Wait for bundle input option #3577
Wait for bundle input option #3577
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3577 +/- ##
=======================================
Coverage 96.35% 96.36%
=======================================
Files 180 181 +1
Lines 6151 6166 +15
Branches 1808 1812 +4
=======================================
+ Hits 5927 5942 +15
Misses 111 111
Partials 113 113
Continue to review full report at Codecov.
|
Thank you for your PR. I see how this is useful for your setup, however I am not sure if we want to make this a core feature. Reasons are mainly architecturally as I think this feature is only ever useful in watch-mode, but now adds quite a bit of additional churn to the already very crowded module loader. And there will be some maintenance cost in the future as well as this possibly makes some upcoming refactorings harder. Why not make it a plugin, something like the following should solve your use case: function waitForInputPlugin() {
return {
name: 'wait-for-input',
async buildStart(options) {
const inputSpecifiers =
typeof options.input === 'string'
? [options.input]
: Array.isArray(options.input)
? options.input
: Object.keys(input);
let lastAwaitedSpecifier = null;
checkSpecifiers: while (true) {
for (const specifier of inputSpecifiers) {
if (await this.resolve(specifier) === null) {
if (lastAwaitedSpecifier !== specifier) {
console.log(`Waiting for input "${specifier}"...`);
lastAwaitedSpecifier = specifier;
}
await new Promise(resolve => setTimeout(resolve, 500));
continue checkSpecifiers;
}
}
break;
}
}
};
} It makes use of the fact that the builtin handler for |
Thank's for the feedback, the plugin approach solve my issue very well. To make this easier to use by others, it might be an option to make this the default behavior when using watch-mode, in this way there will be way less churn. The change will mostly be limited to the let resolveIdResult: ResolveIdResult;
const unresolvedFiles = new Set<string>();
while (
(resolveIdResult = await resolveId(
unresolvedId,
importer,
this.preserveSymlinks,
this.pluginDriver,
null
)) === undefined && this.graph.watcher !== null
) {
if (!unresolvedFiles.has(unresolvedId)) {
this.graph.watcher.emit('event', {
code: 'INPUT_WAIT',
input: unresolvedId
});
unresolvedFiles.add(unresolvedId);
}
await new Promise<void>(resolve => {
setTimeout(resolve, 500);
});
} In this approach the availability of the I'm willing to change the pull request or create a new one, if you are interested, if not i'm happy also. |
I have thought about it a little and I am still somewhat hesitant about this. Nevertheless I see that ease of use is of course valuable. On the other hand, I do not feel good about this being the default behaviour for the pure JS API as well, there may be quite a few edge case scenarios that play into this. rollup/cli/run/commandPlugins.ts Line 10 in 462bff7
Also it is a very well defined environment in that it always runs in Node and there are no downstream tools to consider. The plugin could be injected somewhere here Line 18 in 462bff7
What do you think? With regard to testing, you could take inspiration from the tests here https://github.com/rollup/rollup/tree/master/test/cli/samples/watch |
That sound like a solid solution. I tested it and it is working, in my test the plugin is loaded in the ({ options: configs, warnings } = await loadAndParseConfigFile(configFile!, command));
reloadingConfig = false;
for(const config of configs){
if (config.plugins) {
config.plugins.push(waitForInputPlugin());
}
} I can update the pull request accordingly. For my use case it would be practical to have the option to enable the plugin for build as well. I can still add the plugin to the But maybe my use case is a bit too specialised. |
Then maybe we do it yet again slightly differently, add an option |
That sounds good one small addition, shall we always load the plugin when in watch mode? So the |
IMO if we add an option I would not set it automatically for the reason you mentioned, you cannot get rid of it. If there is a typo in the input name, it might be helpful for the build to fail even in watch-mode instead of "hanging". This will also make this very much a non-breaking change. One can still revisit making this a default for watch-mode with the next major. |
I agree :-), i will update the pull request in the way you suggested. |
I have updated the pull request as suggested. The test are probably not good enough but this is the best i can do. |
Looks good, I'll add some tests |
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.
This is good from my side, unless you have any concerns I will release this tomorrow.
No concerns at all thank you and it was nice working together. |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
I'm not sure about the testing part, if it is not okay like this, than can you please give me some pointers.
Description
Added the
--waitForBundleInput
option. The current version of rollup will generate the "Error: Could not resolve entry module" error when the input file or directory does not exists. With this pull request you can use the--waitForBundleInput
option so rollup will wait for the input files to be created.This will make the usage of rollup in conjunction with other tool easier. For example when you start
tsc --watch
androllup --watch
in parallel. The current version of rollup will generate an error, because the bundle input is not available yet. When using the new--waitForBundleInput
option, rollup can just wait for the bundle input file to be available.Example using rollup when directory or file does not exist
rollup --config --watch rollup v2.10.2 bundles tmp/main.js → dist/bundle.js... [!] Error: Could not resolve entry module (tmp/main.js). Error: Could not resolve entry module (tmp/main.js). at error (./node_modules/rollup/dist/shared/rollup.js:161:30) at ModuleLoader.loadEntryModule (./node_modules/rollup/dist/shared/rollup.js:17547:16) at processTicksAndRejections (internal/process/task_queues.js:97:5) at async Promise.all (index 0) at async Promise.all (index 0) at async getCombinedPromise (./node_modules/rollup/dist/shared/rollup.js:17438:13) at async ModuleLoader.awaitLoadModulesPromise (./node_modules/rollup/dist/shared/rollup.js:17443:9) at async ModuleLoader.addEntryModules (./node_modules/rollup/dist/shared/rollup.js:17339:33) at async Promise.all (index 0) at async Graph.parseModules (./node_modules/rollup/dist/shared/rollup.js:18548:63)
Example using rollup with the new
--waitForBundleInput
option