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

Syntax error with valid JS code #21836

Closed
marvinhagemeister opened this issue Jan 7, 2024 · 7 comments · Fixed by #21853
Closed

Syntax error with valid JS code #21836

marvinhagemeister opened this issue Jan 7, 2024 · 7 comments · Fixed by #21853
Labels
bug Something isn't working correctly

Comments

@marvinhagemeister
Copy link
Contributor

marvinhagemeister commented Jan 7, 2024

Steps to reproduce

Run this code

import Plotly from "npm:[email protected]"
console.log(Plotly)

Error:

Uncaught SyntaxError: Invalid or unexpected token at file:///Users/marvinh/Library/Caches/deno/npm/registry.npmjs.org/plotly.js-dist/2.27.1/plotly.js:245:14

Looks like something breaks in SWC

Version: Deno 1.39.1

@marvinhagemeister
Copy link
Contributor Author

I managed to narrow it down to a small reproduction case

Steps to reproduce:

  1. Clone https://github.com/marvinhagemeister/deno-cjs2
  2. Run deno run -A main.ts

Error:

error: Uncaught SyntaxError: Invalid or unexpected token

The heart of the issue is a commonjs export which starts with a number.

module.exports = {
  "123_foo": "it works"
}

We seem to try to convert this module to an ESM module somewhere but fail to check if the object key would be a valid JS identifier. JS identifiers must not start with a number at which point we run into a syntax error.

@bartlomieju
Copy link
Member

Good find, I think we should see what Node.js does in translation cases like this and adjust accordingly.

@marvinhagemeister
Copy link
Contributor Author

marvinhagemeister commented Jan 8, 2024

Can confirm that we apply an invalid transform.

// input
module.exports = {
  "3d": "it works"
}

// output
import {createRequire as __internalCreateRequire} from "node:module";
const require = __internalCreateRequire(import.meta.url);
const mod = require("/Users/marvinh/dev/test/deno-cjs2/node_modules/foo/index.js");

// this is wrong, identifiers must not start with a number.
export const 3d = mod["3d"];
export default mod;

@marvinhagemeister
Copy link
Contributor Author

Looks like node prints a warning when a cjs export cannot be turned into an esm export:

SyntaxError: Named export '3d' not found. The requested module 'foo' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'foo';

@dsherret
Copy link
Member

dsherret commented Jan 8, 2024

@marvinhagemeister fix just needs to be done here:

deno/ext/node/analyze.rs

Lines 393 to 398 in 43d2ecd

fn is_valid_var_decl(name: &str) -> bool {
// it's ok to be super strict here
name
.chars()
.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '$')
}

@marvinhagemeister
Copy link
Contributor Author

@dsherret Neat! For some reason I completely missed that function and was working on my own. We're skipping a bunch of admittedly very rare cases in our validation logic, but it should be fine.

@dsherret
Copy link
Member

dsherret commented Jan 8, 2024

Yeah, that function is purposefully greedy for simplicitly because in this case false positives aren't a big deal.

marvinhagemeister added a commit that referenced this issue Jan 8, 2024
<!--
Before submitting a PR, please read https://deno.com/manual/contributing

1. Give the PR a descriptive title.

  Examples of good title:
    - fix(std/http): Fix race condition in server
    - docs(console): Update docstrings
    - feat(doc): Handle nested reexports

  Examples of bad title:
    - fix #7123
    - update docs
    - fix bugs

2. Ensure there is a related issue and it is referenced in the PR text.
3. Ensure there are tests that cover the changes.
4. Ensure `cargo test` passes.
5. Ensure `./tools/format.js` passes without changing files.
6. Ensure `./tools/lint.js` passes.
7. Open as a draft PR if your work is still in progress. The CI won't
run
   all steps, but you can add '[ci]' to a commit message to force it to.
8. If you would like to run the benchmarks on the CI, add the 'ci-bench'
label.
-->

Fixes #21836
bartlomieju pushed a commit that referenced this issue Jan 12, 2024
<!--
Before submitting a PR, please read https://deno.com/manual/contributing

1. Give the PR a descriptive title.

  Examples of good title:
    - fix(std/http): Fix race condition in server
    - docs(console): Update docstrings
    - feat(doc): Handle nested reexports

  Examples of bad title:
    - fix #7123
    - update docs
    - fix bugs

2. Ensure there is a related issue and it is referenced in the PR text.
3. Ensure there are tests that cover the changes.
4. Ensure `cargo test` passes.
5. Ensure `./tools/format.js` passes without changing files.
6. Ensure `./tools/lint.js` passes.
7. Open as a draft PR if your work is still in progress. The CI won't
run
   all steps, but you can add '[ci]' to a commit message to force it to.
8. If you would like to run the benchmarks on the CI, add the 'ci-bench'
label.
-->

Fixes #21836
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants