feat: external only Source Phase imports support#6279
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install rollup/rollup#source-phase-externalNotice: Ensure you have installed the latest nightly Rust toolchain. If you haven't installed it yet, please see https://www.rust-lang.org/tools/install to learn how to download Rustup and install Rust. or load it into the REPL: |
Performance report
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6279 +/- ##
=======================================
Coverage 98.77% 98.78%
=======================================
Files 273 274 +1
Lines 10728 10772 +44
Branches 2860 2878 +18
=======================================
+ Hits 10597 10641 +44
Misses 89 89
Partials 42 42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lukastaegert
left a comment
There was a problem hiding this comment.
This actually looks fine to me. The only point, but this can be added independently later, would be to make the phase discoverable by plugins in resolvedId. At the moment, I cannot write a plugin that marks source-phase imports as external automatically, or that resolves source-phase imports to something else, or which emits additional files if such an import is detected. But I can also add this myself.
One question to me would be if it would be useful if plugins could implement a load hook for source phase imports and effectively inline the import to whatever the load hook returns, or if this would not make sense. The idea would be that the use of a non-external source phase import would only throw if no load hook is defined. But this could also be a future consideration.
2a1670a to
a2744aa
Compare
Formally speaking, the phase is supposed to be independent of the plugin system in the sense that the plugin should not change resolution or loading based on the phase value. In the spec, this corresponds to the invariant that the module returned by HostLoadImportedModule should be fully independent of the phase value.
The ideal for a full plugin integration of the phase model here would be that if you load the same moduleId multiple times in multiple phases, the plugin hook only gets told the module is being loaded and nothing about the phase information at all, it would then return a module source for that given module Id, and then that same source could be loaded in any phase. I attempted to lay this groundwork in the other PR - happy to discuss it further! |
My thinking was that it would be useful for a plugin to make an import external based on the phase in resolveId, but we could also not do that. A plugin could also use the phase in resolveId to generate a ModuleSource import instead, which would be effectively another module, without any need for a new hook. But there is no reason to add anything here a priori. People could still write a plugin that manually scans for source phase imports in a transform hook and replaces them on that level. Otherwise for the non-external perspective, I think we still need to rethink this. If you look at JavaScript modules for instance, then if you want to create a source phase representation, it does not make any sense at all to give access to the result of the load hook. There is no guarantee that this has anything to do with the final module source after transforms. Instead, you would need access to the result of the transform hook. On the other hand for the WASM use case, the transform hook would be wrong as that one would convert it to JavaScript. I still think that the load hook should do that, but you are right, the WASM plugin does it differently. I think in order to advance here at all, we should probably think about a concept of "content types" first. But the external-only case could be a good start. |
The source does refer the actual source - for JS the JS source, for Wasm, the Wasm bytes. JS source imports are specified in https://github.com/tc39/proposal-esm-phase-imports, which is currently Stage 2.7 and provides the JS representation for module declarations as well. Here are the slides I presented yesterday at TC39 on this which may be of interest - https://docs.google.com/presentation/d/1inTcnb4hugyAvKrjFX_XHTnxYaPW0rodWwn0VFmlq24/edit?usp=sharing.
Ideally the load hook would be all that is needed - it just returns either wasm bytes or JS source text and then all phases can be correctly represented. That would though assume some kind of first-class Wasm support in core though, which understandly isn't a goal right now, but at the very least that's the model that justifies phase independence and this not just being a custom hook. |
ce1a3ef to
18ecedd
Compare
f0eaf21 to
4cbdfad
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds Source Phase Imports support across Rollup’s parsing and bundling pipeline, with the intentionally limited behavior that source-phase imports are only allowed when they resolve to external modules, and are only emitted for ESM output.
Changes:
- Extend AST parsing/serialization (JS + Rust) to carry an optional
phase(source/defer) onImportDeclarationandImportExpression. - Enforce “source phase imports must be external” for static imports and add new diagnostic codes/URLs.
- Add finaliser behavior to preserve
import source …inesoutput and error for unsupported output formats, plus new tests and docs.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/testHelpers.js | Extends the Acorn test parser to understand import phases. |
| test/function/samples/source-phase-import-error/main.js | New sample: non-external import source should error. |
| test/function/samples/source-phase-import-error/dep.js | New sample dependency for the error test. |
| test/function/samples/source-phase-import-error/_config.js | Asserts new NON_EXTERNAL_SOURCE_PHASE_IMPORT error details. |
| test/function/samples/source-phase-format-unsupported/main.js | New sample: source phase external with non-ES output should error. |
| test/function/samples/source-phase-format-unsupported/_config.js | Asserts new SOURCE_PHASE_FORMAT_UNSUPPORTED generate error. |
| test/form/samples/source-phase-imports-external/main.js | New form sample verifying preserved import source output. |
| test/form/samples/source-phase-imports-external/_expected.js | Expected ESM output including preserved import source. |
| test/form/samples/source-phase-imports-external/_config.js | Form test configuration for the new sample. |
| src/utils/urls.ts | Adds a dedicated docs URL constant for source-phase imports. |
| src/utils/logs.ts | Adds new log codes/messages for source-phase import errors. |
| src/utils/deconflictChunk.ts | Deconflicts naming for source-phase external import bindings. |
| src/utils/convert-ast-strings.ts | Adds source/defer to fixed string table for AST buffers. |
| src/utils/bufferToAst.ts | Decodes optional phase from AST buffers into ESTree nodes/types. |
| src/ModuleLoader.ts | Adds external-only validation for static source-phase imports. |
| src/Module.ts | Tracks source-phase import sources and annotates import descriptions with phase. |
| src/Graph.ts | Skips missing-export warnings for source-phase imports. |
| src/finalisers/umd.ts | Errors on source-phase imports for UMD output via throwOnPhase. |
| src/finalisers/system.ts | Errors on source-phase imports for SystemJS output via throwOnPhase. |
| src/finalisers/shared/throwOnPhase.ts | New helper to raise SOURCE_PHASE_FORMAT_UNSUPPORTED for non-ES formats. |
| src/finalisers/iife.ts | Errors on source-phase imports for IIFE output via throwOnPhase. |
| src/finalisers/es.ts | Emits import source … for dependencies marked as source-phase imports. |
| src/finalisers/cjs.ts | Errors on source-phase imports for CJS output via throwOnPhase. |
| src/finalisers/amd.ts | Errors on source-phase imports for AMD output via throwOnPhase. |
| src/Chunk.ts | Plumbs source-phase import specifiers into rendered dependency metadata. |
| src/ast/variables/ExternalVariable.ts | Adds SOURCE_PHASE_IMPORT sentinel and isSourcePhase flag. |
| src/ast/nodes/ImportExpression.ts | Adds phase field to dynamic import AST node type. |
| src/ast/nodes/ImportDeclaration.ts | Adds phase field to static import AST node type. |
| src/ast/bufferParsers.ts | Parses optional phase from buffers into node instances. |
| scripts/generate-string-constants.js | Updates generator to include source/defer string constants. |
| scripts/generate-buffer-to-ast.js | Adds generator support for OptionalFixedString decoding. |
| scripts/generate-buffer-parsers.js | Adds generator support for OptionalFixedString parsing. |
| scripts/generate-ast-macros.js | Allows macros to handle OptionalFixedString fields. |
| scripts/ast-types.js | Introduces OptionalFixedString field type and adds phase to import nodes. |
| rust/parse_ast/src/convert_ast/converter/string_constants.rs | Adds Rust string IDs for source/defer. |
| rust/parse_ast/src/convert_ast/converter/ast_constants.rs | Extends reserved bytes/offsets for import nodes to store phase. |
| rust/parse_ast/src/convert_ast/converter.rs | Passes SWC Import node into import-expression storage. |
| rust/parse_ast/src/ast_nodes/import_expression.rs | Writes phase into the AST buffer for dynamic imports. |
| rust/parse_ast/src/ast_nodes/import_declaration.rs | Writes phase into the AST buffer for static imports. |
| package.json | Adds acorn-import-phases dev dependency. |
| package-lock.json | Locks acorn-import-phases dependency. |
| docs/es-module-syntax/index.md | Documents source phase import syntax and Rollup limitations. |
| docs/configuration-options/index.md | Documents external requirement for source-phase imports. |
Comments suppressed due to low confidence (1)
src/ast/nodes/ImportExpression.ts:63
phaseis now parsed/stored onImportExpression, but there is no phase-aware behavior during dynamic import resolution/rendering. In particular, source-phase dynamic imports can currently be transformed by non-ES output mechanisms (e.g. torequire(...)in CJS/AMD) or even code-split to internal chunks, which would change semantics and bypass the “external-only” restriction. Consider explicitly rejectingphase === 'source'for non-esoutputs and for internal resolutions, or otherwise implement dedicated handling for source-phase dynamic imports.
export default class ImportExpression extends NodeBase {
declare options: ExpressionNode | null;
inlineNamespace: NamespaceVariable | null = null;
declare phase: 'source' | 'defer' | undefined;
declare source: ExpressionNode;
declare type: NodeType.tImportExpression;
declare sourceAstNode: AstNode;
resolution: Module | ExternalModule | string | null = null;
fd247d6 to
e1d21ea
Compare
Add missing url fields and fix message text to match the updated logSourcePhaseFormatUnsupported and logNonExternalSourcePhaseImport functions in src/utils/logs.ts.
44ecf73 to
7aa374c
Compare
7aa374c to
b54ce80
Compare
b54ce80 to
669d555
Compare
669d555 to
d92359a
Compare
d92359a to
c5660c3
Compare
|
This PR has been released as part of rollup@4.60.0. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
Related issues
Description
This implements source phase imports support similarly to esbuild - supporting the syntax in Rollup's AST, but then only allowing it to be treated as an external import.
This provides one of the major benefits of the Source Phase Imports Stage 3 proposal in enabling cross-platform compiled Wasm loading, while ensuring keeping the implementation minimal. The nice thing about this approach is that the syntax drives the usage with no further configuration necessary apart from the externalization.
Tests are added for the new errors thrown when either outputting for a module format that does not support the source phase or when not marking a source phase import as external.
The AST integration matches Acorn's source phase support in treating the phase as an optional string, which required a new OptionalFixedString AST integration type.
Within the module loader the source phase integration is now fairly straightforward while also laying the groundwork for the defer phase support.