Plugins: plugin-commonjs: dashes in filename result in invalid javascript code

[Haringat]

• Rollup Plugin Name: commonjs
• Rollup Plugin Version: 11.0.0
• Rollup Version: 1.27.13
• Operating System (or Browser): Arch linux
• Node Version: 13.3.0

How Do We Reproduce?

repl.it link: https://repl.it/repls/ElderlySoggyNetworks

Expected Behavior

The plugin creates a valid identifier for the export name

Actual Behavior

the filename "parse-int" will be converted to "_parse-int" (moduleName) and used as identifier. Since a "-" must not occur in identifiers this leads to errors when parsing the resulting code afterwards.

[shellscape]

🎉 thank you for using the new repl.it template!

I tried to run it but am getting an error:

input.js → output/bundle.js...
[!] Error: Unexpected token
parse-int.js (10:14)
 8: // `parseInt` method
 9: // https://tc39.github.io/ecma262/#sec-parseint-string-radix
10: module.exports = FORCED ? function parseInt(string, radix) {
                  ^
11:   var S = String(string).trim();
12:   return nativeParseInt(S, (radix >>> 0) || (hex.test(S) ? 16 : 10));
Error: Unexpected token
    at error (/home/runner/node_modules/rollup/dist/rollup.js:5365:30)
    at Module.error (/home/runner/node_modules/rollup/dist/rollup.js:9708:9)
    at tryParse (/home/runner/node_modules/rollup/dist/rollup.js:9617:16)
    at Module.setSource (/home/runner/node_modules/rollup/dist/rollup.js:9935:33)
    at Promise.resolve.catch.then.then.then (/home/runner/node_modules/rollup/dist/rollup.js:12205:20)

If this works for you, I can let the repl.it folks know something isn't right. Could you take another quick look?

[vincentriemer]

@shellscape that's actually the same error that's the subject of this issue. The error is confusing because it's displaying the un-transformed code and mapping the line of the transformed code to to it. That line where the unexpected token error occurs in the transformed code is instead something like:

var _parse-int = FORCED ? function parseInt(string, radix) {
  var S = String(string).trim();
  return nativeParseInt(S, (radix >>> 0) || (hex.test(S) ? 16 : 10));

...and it is the identifier _parse-int that's created by the CommonJS plugin which is triggering the unexpected token error as identifiers can't have dashes in them.

[LarsDenBakker]

I ran into the same issue, but couldnt figure out what could be the cause of it.

Great that you were able to make a minimal reproduction

[vincentriemer]

I was able to fix the issue by making the following change on line 25 in transform.js

- let deconflicted = identifier;
+ let deconflicted = makeLegalIdentifier(identifier);

I'm not familiar enough with the plugin's codebase to know if this is the exact place to make the fix but I hope it helps with the investigation.

[shellscape]

@vincentriemer @LarsDenBakker thanks for that clarification. We'll have to take a look at what's being returned from commonjs to triage this I suppose, since I'm unable to get a proper bundle to output to disk.

^^ we replied at the same time. Given your last reply, we should take a look at makeLegalIdentifier and see what it's not accounting for there.

[vincentriemer]

Ah maybe I didn't show enough context, what I changed was making the deconflict function utilize makeLegalIdentifier where it previously wasn't. I don't think there are any issues in makeLegalIdentifier.

[shellscape]

No worries, I'm distracted right now by a small child 😄

[shellscape]

@vincentriemer on your suggestion I've made that change in the fix/commonjs/dash-name branch and all tests pass. I need to come up with a good test for this and then I'll PR it. thanks for your help on this.