-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Comments
I managed to narrow it down to a small reproduction case Steps to reproduce:
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. |
Good find, I think we should see what Node.js does in translation cases like this and adjust accordingly. |
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; |
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'; |
@marvinhagemeister fix just needs to be done here: Lines 393 to 398 in 43d2ecd
|
@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. |
Yeah, that function is purposefully greedy for simplicitly because in this case false positives aren't a big deal. |
<!-- 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
<!-- 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
Steps to reproduce
Run this code
Error:
Looks like something breaks in SWC
Version: Deno 1.39.1
The text was updated successfully, but these errors were encountered: