Refactor codebase from CommonJS to ESM #177
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Find/replace: ``` const (.*?) = require\((.*?)\); import $1 from $2; ```
Find replace:
```
module\.exports = \{
export {
```
```
module\.exports = (.*?);
export default $1;
```
MadLittleMods
added
the
T-Task
MadLittleMods
force-pushed
the
madlittlemods/commonjs-to-esm
branch
from
53991be
to
bcb02f8
Compare
MadLittleMods
commented
Apr 22, 2023
Comment on lines
+101
to
+126
| const hydrogenRenderModule = new vm.SourceTextModule(hydrogenRenderScriptCode, { | ||
| identifier: path.basename(vmRenderScriptFilePath), | ||
| context: vmContext, | ||
| }); | ||
| // TODO: This is crazy compared to the normal `vm.Script(...)` API and this doesn't | ||
| // work at the moment because it only handles ESM imports or exports, idk. Too much BS | ||
| // if I just want the behavior that Node.js already provides by default. | ||
| // | ||
| // And the only reason we need to use `vm.SourceTextModule` is because we need see | ||
| // `SyntaxError: Cannot use import statement outside a module` in the script we try to | ||
| // run with `vm.Script(...)` and `vm.runInContext(vmContext);`. | ||
| // | ||
| // XXX: This is crazy to handle on our own! | ||
| // https://github.com/nodejs/node/issues/31234 Also see | ||
| // https://github.com/nodejs/node/issues/35848 | ||
| await hydrogenRenderModule.link((specifier, referencingModule) => { | ||
| return new Promise(async function (resolve, reject) { | ||
| const mod = await import(specifier); | ||
| resolve( | ||
| new vm.SyntheticModule(['default'], function () { | ||
| this.setExport('default', mod.default); | ||
| }) | ||
| ); | ||
| }); | ||
| }); | ||
| // Note: The VM does not exit after the result is returned here and is why | ||
| // this should be run in a `child_process` that we can exit. | ||
| const vmResult = hydrogenRenderScript.runInContext(vmContext); | ||
| // Wait for everything to render | ||
| // (waiting on the promise returned from the VM render script) | ||
| await vmResult; | ||
| await hydrogenRenderModule.evaluate(); |
There was a problem hiding this comment.
This area will be the downfall of this migration 😢
With the previous new vm.Script(code).runInContext(vmContext), we see SyntaxError: Cannot use import statement outside a module after the ESM refactor
Which prompted an update to new vm.SourceTextModule(...) ... but this API surface is crazy and expects to implement our own linker when I just want default Node.js behavior.
- Simplify vm.Module nodejs/node#43899
- Provide built-in implementations of module linker, initializeImportMeta, importModuleDynamically nodejs/node#31234
- Link modules with paths in module.link(linker) nodejs/node#35848
- Linking cycle with
vm.SourceTextModulenodejs/help#3535
The linker I tried to copy from the above issues isn't sufficient because it doesn't handle the menagerie of Node.js specific behavior or event CommonJS modules. We would have to do something much more robust like these (crazy):
- https://github.com/jestjs/jest/blob/6e95bdcbf9e5c2e354f277d0db6e4dff1ad2c43f/packages/jest-runtime/src/index.ts#L474-L513
- https://github.com/cloudflare/miniflare/blob/2d0fbc624bfa46a31f6c47f342a96ce7fdddebd2/packages/runner-vm/src/linker.ts#L57-L167
The new vm.Script(...) API (or some API) should handle all of the complexity of whether I used import or require syntax and just load dependencies (sane defaults please).
There was a problem hiding this comment.
(giving up for now ⏩)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor codebase from CommonJS to ESM
Spawning from the
gulp-revdependency in #175 being ESM which kinda necessitates that we also switch over.Dev notes
See https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c