Skip to content
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

No fallback for Register when entrypoint is a commonjs vscode extension #795

Closed
psychobolt opened this issue Jun 30, 2024 · 5 comments · Fixed by #797
Closed

No fallback for Register when entrypoint is a commonjs vscode extension #795

psychobolt opened this issue Jun 30, 2024 · 5 comments · Fixed by #797

Comments

@psychobolt
Copy link

psychobolt commented Jun 30, 2024

Hi, trying to execute my custom runtime with a vscode extension to support loading ESM files with register hook v1.10. However, I am getting this error in outputs:

[Info  - 10:13:43 AM] ESLint server is starting.
URL:file:///Users/mitran/.vscode/extensions/dbaeumer.vscode-eslint-3.0.10/server/out/eslintServer.js

node:internal/modules/run_main:129
    triggerUncaughtException(
    ^
Error: Failed to convert JavaScript value `Undefined` into rust type `String`
    at Compiler.<anonymous> (/Users/mitran/Projects/vite-storybook-boilerplate/.yarn/unplugged/@swc-core-virtual-d7e358b2c0/node_modules/@swc/core/index.js:218:33)
    at Generator.next (<anonymous>)
    at /Users/mitran/Projects/vite-storybook-boilerplate/.yarn/unplugged/@swc-core-virtual-d7e358b2c0/node_modules/@swc/core/index.js:31:71
    at new Promise (<anonymous>)
    at __awaiter (/Users/mitran/Projects/vite-storybook-boilerplate/.yarn/unplugged/@swc-core-virtual-d7e358b2c0/node_modules/@swc/core/index.js:27:12)
    at Compiler.transform (/Users/mitran/Projects/vite-storybook-boilerplate/.yarn/unplugged/@swc-core-virtual-d7e358b2c0/node_modules/@swc/core/index.js:203:16)
    at transform (/Users/mitran/Projects/vite-storybook-boilerplate/.yarn/unplugged/@swc-core-virtual-d7e358b2c0/node_modules/@swc/core/index.js:352:21)
    at transform (/Users/mitran/Projects/vite-storybook-boilerplate/.yarn/__virtual__/@swc-node-core-virtual-849eb160d1/3/.yarn/berry/cache/@swc-node-core-npm-1.13.1-7bd6e51ef2-10c0.zip/node_modules/@swc-node/core/lib/index.js:71:33)
    at compile (/Users/mitran/Projects/vite-storybook-boilerplate/.yarn/unplugged/@swc-node-register-virtual-ef6475fd81/node_modules/@swc-node/register/lib/register.js:81:37)
    at load (file:///Users/mitran/Projects/vite-storybook-boilerplate/.yarn/unplugged/@swc-node-register-virtual-ef6475fd81/node_modules/@swc-node/register/esm/esm.mjs:183:28) {
  code: 'StringExpected'
}

Node.js v20.15.0

What would be executed for example would be ESLINT_USE_FLAT_CONFIG=true node bin/swc-node.js "../../.vscode/extensions/dbaeumer.vscode-eslint-3.0.10/server/out/eslintServer.js" -c eslint.config.ts.

Analysis

It seems that any commonjs file would cause the same error. For example I've created a simple test:

// foo.cjs
module.exports = {
  foo: 'bar'
};
node --import @swc-node/register ./foo.cjs

Would recieve the same error:

node:internal/modules/run_main:129
    triggerUncaughtException(
    ^
Error: Failed to convert JavaScript value `Undefined` into rust type `String`

Seeing here we shortcircuit and fails to compile when source is undefined. I would assume we need a similar fallback, which fixed the Yarn loader case . I have my workaround which works in my case:

diff --git a/esm/esm.mjs b/esm/esm.mjs
index 1e7429abec21338477f904b17a35161b75d01e19..f0a3db97959040ea925240ed0f7e38bba4dfb0e6 100644
--- a/esm/esm.mjs
+++ b/esm/esm.mjs
@@ -175,10 +175,14 @@ export const load = async (url, context, nextLoad) => {
         debug('loaded: internal format', url);
         return nextLoad(url, context);
     }
+    // workaround for a vscode extension, fallback. See https://github.com/microsoft/vscode/issues/130367
+    if (url.indexOf('.vscode/extensions') > 0) {
+        return nextLoad(url, context);
+    }
     const { source, format: resolvedFormat } = await nextLoad(url, context);
@psychobolt psychobolt changed the title Register fails when entrypoint is a commonjs vscode extension No fallback when register fails when entrypoint is a commonjs vscode extension Jun 30, 2024
@psychobolt psychobolt changed the title No fallback when register fails when entrypoint is a commonjs vscode extension No fallback for register fails when entrypoint is a commonjs vscode extension Jun 30, 2024
@psychobolt psychobolt changed the title No fallback for register fails when entrypoint is a commonjs vscode extension No fallback for Register when entrypoint is a commonjs vscode extension Jun 30, 2024
@fargito
Copy link
Contributor

fargito commented Jul 1, 2024

I have the same issue, however I think it is not specific to VSCode extension. I encounter it when I import a commonjs file with node. I opened #796 but I think my fix was incorrect, so I closed it.

@fargito
Copy link
Contributor

fargito commented Jul 1, 2024

I'm even wondering if commonjs files should be skipped altogether?

in packages/register/esm.mts

export const load: LoadHook = async (url, context, nextLoad) => {
  debug('load', url, JSON.stringify(context))

  if (url.startsWith('data:')) {
    debug('skip load: data url', url)

    return nextLoad(url, context)
  }

  if (['builtin', 'json', 'wasm'].includes(context.format)) {
    debug('loaded: internal format', url)
    return nextLoad(url, context)
  }

+  if (context.format === "commonjs") {
+    debug('skip load: commonjs file', url);
+    return nextLoad(url, context);
+  }

  const { source, format: resolvedFormat } = await nextLoad(url, context)

  debug('loaded', url, resolvedFormat)

  const code = !source || typeof source === 'string' ? source : Buffer.from(source).toString()
  const compiled = await compile(code, url, tsconfigForSWCNode, true)

  debug('compiled', url, resolvedFormat)

  return addShortCircuitSignal({
    // for lazy: ts-node think format would undefined, actually it should not, keep it as original temporarily
    format: resolvedFormat,
    source: compiled,
  })
}

WDYT @Brooooooklyn?

@yeliex
Copy link
Contributor

yeliex commented Jul 2, 2024

I'm even wondering if commonjs files should be skipped altogether?

definitely no. import commonjs in esm is a very common case

I would assume we need a similar fallback, which fixed the Yarn loader #772 (comment)

A workaround should be. but ignoring by path prefix is too rude. I would look at this, maybe we could have a better way.

@yeliex
Copy link
Contributor

yeliex commented Jul 2, 2024

could you please provide a minimal reproduction

@psychobolt
Copy link
Author

Got a different error on this sandbox, but fails on commonjs script. Possibly related?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants