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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed imports/exports that are illegal identifiers in the es output #4939
Changes from 2 commits
a56d936
7d8a186
09208df
76fcbbc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
const validIdentifier = /^(?!\d)[\w$]+$/; | ||
|
||
export function isValidIdentifier(name: string): boolean { | ||
return validIdentifier.test(name); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
module.exports = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While chunkingForm is the only format to support multi-file output verification, this is not needed here and I would recommend to move this to a "form" test so that it will also verify UMD and IIFE output formats. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved this to the requested location |
||
description: 'correctly handles illegal identifiers in exports', | ||
options: { | ||
input: ['main'] | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
define(['exports'], (function (exports) { 'use strict'; | ||
|
||
const legal = 10; | ||
|
||
exports.legal = legal; | ||
exports["馃敟illegal"] = legal; | ||
|
||
})); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
'use strict'; | ||
|
||
const legal = 10; | ||
|
||
exports.legal = legal; | ||
exports["馃敟illegal"] = legal; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const legal = 10; | ||
|
||
export { legal, legal as "馃敟illegal" }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
System.register([], (function (exports) { | ||
'use strict'; | ||
return { | ||
execute: (function () { | ||
|
||
const legal = 10; exports({ legal: legal, '馃敟illegal': legal }); | ||
|
||
}) | ||
}; | ||
})); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export const legal = 10; | ||
|
||
export { legal as '馃敟illegal' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is the only place that has to be fixed here as no other code refers to
specifier.exported
and local specifiers (and reexporting ones) have to be legal already. It would be worth rechecking if I'm right thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be at least two other places to check, see this example:
https://rollupjs.org/repl/?pr=4939&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QiclM0EnJTIwYXMlMjBiYXolN0QlMjBmcm9tJTIwJ2V4dGVybmFsJyUzQiU1Q25jb25zb2xlLmxvZyhiYXopJTNCJTVDbmNvbnN0JTIwZm9vJTIwJTNEJTIwJ2JhciclM0IlNUNuZXhwb3J0JTIwJTdCZm9vJTIwYXMlMjAnJTJCJyU3RCU1Q25leHBvcnQlMjAlN0JiYXIlMjBhcyUyMCctJyUyQyUyMCclMkYnJTdEJTIwZnJvbSUyMCdleHRlcm5hbCclM0IlNUNuJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlMkMlMjJuYW1lJTIyJTNBJTIybWFpbi5qcyUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJvdXRwdXQlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlcyUyMiU3RCUyQyUyMnRyZWVzaGFrZSUyMiUzQXRydWUlN0QlN0Q=
Especially for imports, it means we need to ensure that illegal names are not used for local identifiers.
Not sure if you want to tackle all of them, I can certainly help in identifying those places. The most difficult is probably the local name of the external import. Maybe this can be fixed in
getSafeName.ts
.