refactor: Use ES modules for internal runtime code #17648
Conversation
| @@ -0,0 +1,4 @@ | |||
| error: Uncaught (in promise) TypeError: Cannot load internal module from external code | |||
There was a problem hiding this comment.
How does CLI get to the point of this error? This error is thrown by the internal module loader, not the cli loader. What is up with that?
There was a problem hiding this comment.
That's a good point @crowlKats, we shouldn't get an error from the CLI loader, not the internal loader.
There was a problem hiding this comment.
Why was this resolved? AFAICT, the error is still coming from the internal loader. Why is the internal loader used at runtime in CLI?
There was a problem hiding this comment.
@lucacasonato it's not coming from internal loader - I disabled dynamic imports of internal modules completely in core/bindings.rs - so we're not even hitting module system, nor any of the loaders.
There was a problem hiding this comment.
Can we do the same for resolution in static loading?
There was a problem hiding this comment.
static loading is erroring before currently, because the FileFetcher is erroring with invalid scheme
There was a problem hiding this comment.
I think we could do that somewhere in core/modules.rs - essentially you're asking to hard disable loading of internal modules in cases where we are running from a snapshot?
There was a problem hiding this comment.
essentially you're asking to hard disable loading of internal modules in cases where we are running from a snapshot?
we already are doing that though, no? the InternalModuleLoader isnt used by cli; its not used if one is loading a snapshot but not creating one
There was a problem hiding this comment.
Please do the enforcement at core level in a follow up as Bartek described. CLI is not the only loader implementation. Not all loaders enforce a given scheme during resolution.
There was a problem hiding this comment.
There will be several clean ups needed after landing this PR, but this is a major effort that brings us very close to be able to snapshot TypeScript modules.
We should land it ASAP, to catch any potential problems and have time to fix them before the next release.
LGTM!
…17694) Addressing Luca concerns from #17648 (comment)
This PR refactors all internal js files (except core) to be written as ES modules. `__bootstrap`has been mostly replaced with static imports in form in `internal:[path to file from repo root]`. To specify if files are ESM, an `esm` method has been added to `Extension`, similar to the `js` method. A new ModuleLoader called `InternalModuleLoader` has been added to enable the loading of internal specifiers, which is used in all situations except when a snapshot is only loaded, and not a new one is created from it. --------- Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>
This PR refactors all internal js files (except core) to be written as ES modules.
__bootstraphas been mostly replaced with static imports in form ininternal:[path to file from repo root].To specify if files are ESM, an
esmmethod has been added toExtension, similar to thejsmethod.A new ModuleLoader called
InternalModuleLoaderhas been added to enable the loading of internal specifiers, which is used in all situations except when a snapshot is only loaded, and not a new one is created from it.