feat(compile): Enable multiple roots for a standalone module graph #17663

andreubotella · 2023-02-05T23:06:26Z

This change will enable dynamic imports and web workers to use modules not reachable from the main module, by passing a list of extra side module roots as options to deno compile.

This PR builds on top of #17657, which adds support for web workers to deno compile.

The option name --extra-side-modules is up for bikeshedding, as well as the exact way to specify such list of modules. I suspect this list could be added to deno.json.

Closes #17908

cli/tests/testdata/compile/dynamic_imports/main.js

aapoalas

Minor nitpicks and bikesheddings aside, looks good to me and really useful.

Statically analysable dynamic imports being picked up into the module graph automatically would be a really good feature to get working, as it would make both deno compile and Deno Deploy users quite happy indeed. For Workers I don't think it would work though, since it's possible to override the Worker name can be overridden both by local shadowing and globally by overriding globalThis.Worker.

cli/args/flags.rs

cli/tests/testdata/compile/dynamic_imports/main.js

cli/tests/testdata/compile/workers/not_in_module_map.js

dsherret

We should be able to do it without needing --extra-side-modules.

Edit: let's wait on denoland/eszip#115

andreubotella · 2023-02-11T17:24:40Z

We should be able to do it without needing --extra-side-modules.

Some flag would still be needed for non-analyzable dynamic imports, and for workers.

dsherret · 2023-02-11T17:28:20Z

Sorry, I read too fast and missed about workers. I think that's reasonable for workers and in that case maybe we can just merge this without the analyzing of statically analyzable dynamic imports. For some reason, all our walking/traversing of the graph in the CLI skips over statically analyzable dynamic imports, but they're in the ModuleGraph. I'll review this now.

aapoalas · 2023-02-11T17:30:18Z

I agree with @andreubotella that the flag would be useful for non-analysable imports but... It's actually also possible to do that without the flag:

const EXTRA_SIDE_MODULES = [
  () => import("./side-module-1.ts"),
  () => import("../root-side-module.ts"),
];

It's probably a matter of taste which is more elegant: Use the existing language features that we have, or add a configuration flag or option.

andreubotella · 2023-02-11T17:34:08Z

Sorry, I read too fast and missed about workers. I think that's reasonable for workers and in that case maybe we can just merge this without the analyzing of statically analyzable dynamic imports. For some reason, all our walking/traversing of the graph in the CLI skips over statically analyzable dynamic imports, but they're in the ModuleGraph. I'll review this now.

Thanks. You should review commit 4d8e192, since the previous ones come from #17657. I built this PR off of that one not because the actual code changes need to be based on that PR, but because that way I can add to one of the worker tests from that PR.

I agree with @andreubotella that the flag would be useful for non-analysable imports but... It's actually also possible to do that without the flag:
const EXTRA_SIDE_MODULES = [
  () => import("./side-module-1.ts"),
  () => import("../root-side-module.ts"),
];
It's probably a matter of taste which is more elegant: Use the existing language features that we have, or add a configuration flag or option.

This seems like a hard-to-explain non-obvious hack, if you're reading the code. It feels a lot cleaner to me to say "deno compile will try to detect dynamic imports, but it can't always, so use this flag just in case".

nayeemrmn · 2023-02-11T20:01:47Z

For some reason, all our walking/traversing of the graph in the CLI skips over statically analyzable dynamic imports

Not sure if you're talking about somewhere specific, but the logic of skipping over (even statically analyzable) dynamic imports exists for example in startup graph validation because we don't want to elevate dynamic module slot errors until the runtime import of that module happens.

dsherret · 2023-02-11T20:13:11Z

Not sure if you're talking about somewhere specific, but the logic of skipping over (even statically analyzable) dynamic imports exists for example in startup graph validation because we don't want to elevate dynamic module slot errors until the runtime import of that module happens.

Yeah, why is that done? I was thinking that maybe someone would be downloading code on the fly, but that wouldn't work. It seems to shift an error that could be spotted at startup to being surfaced later on. Anyway, in this case we can make it work by updating eszip to not skip over dynamic imports: denoland/eszip#115

nayeemrmn · 2023-02-11T20:16:21Z

Yeah, why is that done? I was thinking that maybe someone would be downloading code on the fly, but that wouldn't work. It seems to shift an error that could be spotted at startup to being surfaced later on.

Because the import might not happen, and even be conditioned on whether or not it would error:

if (fs.exists("plugin.ts")) {
  await import("./plugin.ts")
}

Yeah the eszip patch should be sufficient for the deno compile packaging issue

dsherret · 2023-02-11T20:19:26Z

@nayeemrmn that makes sense. Thanks for the background!

nayeemrmn · 2023-02-11T20:37:27Z

Re flag name, the term 'side module' in this context seems too colloquial doesn't it? --imports, --include or even just --modules would be better IMO.

aapoalas · 2023-02-12T17:18:45Z

Re flag name, the term 'side module' in this context seems too colloquial doesn't it? --imports, --include or even just --modules would be better IMO.

I would vote for --import or --include.

aapoalas · 2023-02-12T17:22:43Z

Potentially related and weird errors in in WPT tests for workers:

/html/webappapis/atob/base64.any.worker.html


file result: ok. 380 passed; 0 failed; 0 expected failure; total 380

error: Uncaught (in worker "") TypeError: globalThis_.addEventListener is not a function
    at promiseRejectMacrotaskCallback (internal:cli/runtime/js/99_main.js:376:17)
----------------------------------------
/webidl/ecmascript-binding/global-immutable-prototype.any.worker.html

test stderr:
error: Uncaught (in worker "") TypeError: globalThis_.addEventListener is not a function
    at promiseRejectMacrotaskCallback (internal:cli/runtime/js/99_main.js:376:17)

file result: failed. runner failed during test

First one doesn't fail but it's a bit suspicious. Coming from the 99_main.js file:

    // Add a callback for "error" event - it will be dispatched
    // if error is thrown during dispatch of "unhandledrejection"
    // event.
    globalThis_.addEventListener("error", errorEventCb);

dsherret · 2023-02-12T18:41:38Z

@aapoalas that one is #17405

cli/tests/integration/compile_tests.rs

cli/args/flags.rs

denoland/eszip#115 added support for statically-analyzed dynamic imports in eszip, which made `deno compile` support dynamic imports starting from #17858. This PR adds a test for it. ---- This test is adapted from PR #17663. Closes #17908

andreubotella · 2023-03-05T03:05:21Z

The dynamic_import test from this PR was split into #18017, and I've now updated the PR and added a new version of that test for dynamic imports that can't be statically analyzed. This is tested by reading the import path from the filesystem.

denoland/eszip#115 added support for statically-analyzed dynamic imports in eszip, which made `deno compile` support dynamic imports starting from #17858. This PR adds a test for it. ---- This test is adapted from PR #17663. Closes #17908

bartlomieju · 2023-03-14T13:32:53Z

Discussed during CLI team meeting, we are in favor of landing this PR and going with a flag appears to be a better solution - it's easier to explain and document than requiring statically analyzable dynamic imports. I'm sure we'll bike shed on the flag name a bit. Additional benefit is that it gives a clear way to move this option to the configuration file in the future.

This change will enable dynamic imports and web workers to use modules not reachable from the main module, by passing a list of extra side module roots as options to `deno compile`.

andreubotella · 2023-03-14T19:14:33Z

Rebased to not depend or sit on top of on #17657 anymore.

This change enables the use of web workers in binaries produced by `deno compile`. This requires the main module to be present in the compiled module graph, which is why if it is not already, it must be explicitly added as an additional root (see denoland#17663).

aapoalas

Should add a help comment to the command line argument, otherwise looks ready for merge.

cli/tests/testdata/compile/dynamic_imports/import_path

cli/args/flags.rs

cli/tests/testdata/compile/dynamic_imports/main_unanalyzable.ts

bartlomieju · 2023-03-15T00:00:20Z

cli/args/flags.rs

+          "Includes an additional module in the compiled executable's module \
+    graph. Use this flag if a dynamically imported module or a web worker main \
+    module fails to load in the executable. This flag can be passed multiple \
+    times, to include multiple additional modules.",


Can I do comma separated list of urls like: --side-module=https://deno.land/std/flags/mod.ts,https://deno.land/std/version.ts?

Commas are valid in URLs and filenames.

Hm, I thought we already supported comma separated lists in other flags, but maybe I'm wrong here

cli/graph_util.rs

bartlomieju

I tested it locally with a bunch of dynamic imports and while having to specify --side-module= multiple times is not super nice it does the job very well. I'm sure this functionality will unblock many new use cases for users and I'm strongly in favor of landing it. We can try to further iterate on the flag name and potentially making it more ergonomic, but it seems worth landing as is already.

Awesome work as always @andreubotella, LGTM.

cli/args/flags.rs

bartlomieju

LGTM

cli/tests/testdata/compile/dynamic_imports/main_unanalyzable.ts

dsherret

LGTM too once the timeout is lowered. Thanks for contributing this and the other worker PR! Will be nice to have.

nayeemrmn · 2023-03-17T09:40:08Z

cli/args/flags.rs

@@ -908,6 +909,20 @@ fn compile_subcommand<'a>() -> Command<'a> {
  runtime_args(Command::new("compile"), true, false)
    .trailing_var_arg(true)
    .arg(script_arg().required(true))
+    .arg(
+      Arg::new("side-module")


Was flag name settled? Me and @aapoalas suggested --import which is less colloquial than --side-module.

No - it's still up to debate/bike-shedding, but I'm fine changing the name in a follow up. I'm not sure I like --import better than --side-module, seems more ambigious.

--include ? Shorter is better here...

I'm fine with --include too

Yeah --include was our next one #17663 (comment), sounds perfect if it might include assets as well.

ok let's switch to --include

As far as I'm aware, including assets in the binary is a very different thing than including a module in the module graph. Though I guess you could reconstruct the original TS file from the source map.

In any case, if you agree that the flag name should be --include, I'm fine with that.

littledivy

Tangential: the flag could potentially be reused to define file system assets (ffi dylib / game assets / worker source) to be included in the compiled binary? It is a very useful feature to have IMO.

bartlomieju · 2023-03-17T17:29:59Z

Tangential: the flag could potentially be reused to define file system assets (ffi dylib / game assets / worker source) to be included in the compiled binary? It is a very useful feature to have IMO.

I'm not sure eszip is able to handle these kinds of files, but if it can, we should explore this possibility.

In any case, if you agree that the flag name should be --include, I'm fine with that.

@andreubotella let's go with --include.

andreubotella · 2023-03-18T15:51:27Z

Flag renamed.

bartlomieju

LGTM, thank you @andreubotella!

bartlomieju added this to the 1.31 milestone Feb 6, 2023

dsherret reviewed Feb 7, 2023

View reviewed changes

cli/tests/testdata/compile/dynamic_imports/main.js Outdated Show resolved Hide resolved

andreubotella mentioned this pull request Feb 9, 2023

chore: Reduce contention when using EszipV2 from multiple threads denoland/eszip#108

Closed

andreubotella force-pushed the standalone-side-modules branch from 15e8c12 to 4d8e192 Compare February 10, 2023 12:49

aapoalas approved these changes Feb 11, 2023

View reviewed changes

cli/args/flags.rs Show resolved Hide resolved

cli/tests/testdata/compile/dynamic_imports/main.js Outdated Show resolved Hide resolved

cli/tests/testdata/compile/workers/not_in_module_map.js Outdated Show resolved Hide resolved

andreubotella mentioned this pull request Feb 11, 2023

feat(compile): Add support for web workers in standalone mode #17657

Merged

dsherret requested changes Feb 11, 2023

View reviewed changes

andreubotella force-pushed the standalone-side-modules branch from d91c285 to 7db57ef Compare February 11, 2023 20:14

andreubotella force-pushed the standalone-side-modules branch from 55cde30 to 0df193a Compare February 12, 2023 00:21

aapoalas mentioned this pull request Feb 12, 2023

lsp: include all modules in language server scope for a enabled folder automatically #11190

Closed

dsherret reviewed Feb 12, 2023

View reviewed changes

cli/tests/integration/compile_tests.rs Outdated Show resolved Hide resolved

cli/args/flags.rs Outdated Show resolved Hide resolved

bartlomieju removed this from the 1.31 milestone Feb 23, 2023

andreubotella mentioned this pull request Mar 5, 2023

test(compile): Add a test for dynamic imports in deno compile #18017

Merged

feat(compile): Enable multiple roots for a standalone module graph

1a4d647

This change will enable dynamic imports and web workers to use modules not reachable from the main module, by passing a list of extra side module roots as options to `deno compile`.

andreubotella force-pushed the standalone-side-modules branch from c731dfd to 1a4d647 Compare March 14, 2023 19:13

aapoalas requested changes Mar 14, 2023

View reviewed changes

cli/tests/testdata/compile/dynamic_imports/import_path Outdated Show resolved Hide resolved

cli/args/flags.rs Outdated Show resolved Hide resolved

Add help text for the --side-module argument

29b957d

bartlomieju reviewed Mar 15, 2023

View reviewed changes

Merge branch 'main' into standalone-side-modules

78bc259

bartlomieju reviewed Mar 16, 2023

View reviewed changes

cli/args/flags.rs Outdated Show resolved Hide resolved

Update cli/args/flags.rs

bb2d7b9

bartlomieju approved these changes Mar 16, 2023

View reviewed changes

dsherret reviewed Mar 16, 2023

View reviewed changes

cli/tests/testdata/compile/dynamic_imports/main_unanalyzable.ts Outdated Show resolved Hide resolved

Merge branch 'main' into standalone-side-modules

cdce5b2

dsherret approved these changes Mar 17, 2023

View reviewed changes

Change timeout in dynamic import tests to 0

7744e26

nayeemrmn reviewed Mar 17, 2023

View reviewed changes

littledivy reviewed Mar 17, 2023

View reviewed changes

andreubotella added 2 commits March 18, 2023 16:40

Merge branch 'main' into standalone-side-modules

52bde28

Rename flag to --include

79d17a0

aapoalas approved these changes Mar 18, 2023

View reviewed changes

Merge branch 'main' into standalone-side-modules

8ac9c11

bartlomieju approved these changes Mar 18, 2023

View reviewed changes

bartlomieju merged commit b64ec79 into denoland:main Mar 18, 2023

andreubotella deleted the standalone-side-modules branch March 18, 2023 23:43

feat(compile): Enable multiple roots for a standalone module graph #17663

feat(compile): Enable multiple roots for a standalone module graph #17663

Conversation

andreubotella commented Feb 5, 2023 • edited by dsherret Loading

aapoalas left a comment

Choose a reason for hiding this comment

dsherret left a comment • edited Loading

Choose a reason for hiding this comment

andreubotella commented Feb 11, 2023

dsherret commented Feb 11, 2023

aapoalas commented Feb 11, 2023 • edited Loading

andreubotella commented Feb 11, 2023 • edited Loading

nayeemrmn commented Feb 11, 2023

dsherret commented Feb 11, 2023

nayeemrmn commented Feb 11, 2023 • edited Loading

dsherret commented Feb 11, 2023

nayeemrmn commented Feb 11, 2023

aapoalas commented Feb 12, 2023

aapoalas commented Feb 12, 2023

dsherret commented Feb 12, 2023

andreubotella commented Mar 5, 2023

bartlomieju commented Mar 14, 2023

andreubotella commented Mar 14, 2023 • edited Loading

aapoalas 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

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

dsherret 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

andreubotella Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

littledivy left a comment

Choose a reason for hiding this comment

bartlomieju commented Mar 17, 2023

andreubotella commented Mar 18, 2023

bartlomieju left a comment

Choose a reason for hiding this comment

andreubotella commented Feb 5, 2023 •

edited by dsherret

Loading

dsherret left a comment •

edited

Loading

aapoalas commented Feb 11, 2023 •

edited

Loading

andreubotella commented Feb 11, 2023 •

edited

Loading

nayeemrmn commented Feb 11, 2023 •

edited

Loading

andreubotella commented Mar 14, 2023 •

edited

Loading

andreubotella Mar 17, 2023 •

edited

Loading