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
Fixed imports/exports that are illegal identifiers in the es output #4939
Fixed imports/exports that are illegal identifiers in the es output #4939
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/finalisers/es.ts
Outdated
@@ -131,7 +135,11 @@ function getExportBlock(exports: ChunkExports, { _, cnst }: GenerateCodeSnippets | |||
exportDeclaration.push( | |||
specifier.exported === specifier.local | |||
? specifier.local | |||
: `${specifier.local} as ${specifier.exported}` | |||
: `${specifier.local} as ${ |
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 though
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.
There should be at least two other places to check, see this example:
https://rollupjs.org/repl/?pr=4939&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QiclM0EnJTIwYXMlMjBiYXolN0QlMjBmcm9tJTIwJ2V4dGVybmFsJyUzQiU1Q25jb25zb2xlLmxvZyhiYXopJTNCJTVDbmNvbnN0JTIwZm9vJTIwJTNEJTIwJ2JhciclM0IlNUNuZXhwb3J0JTIwJTdCZm9vJTIwYXMlMjAnJTJCJyU3RCU1Q25leHBvcnQlMjAlN0JiYXIlMjBhcyUyMCctJyUyQyUyMCclMkYnJTdEJTIwZnJvbSUyMCdleHRlcm5hbCclM0IlNUNuJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlMkMlMjJuYW1lJTIyJTNBJTIybWFpbi5qcyUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJvdXRwdXQlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlcyUyMiU3RCUyQyUyMnRyZWVzaGFrZSUyMiUzQXRydWUlN0QlN0Q=
- imports
- reexports
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
.
Codecov Report
@@ Coverage Diff @@
## master #4939 +/- ##
=======================================
Coverage 98.98% 98.98%
=======================================
Files 222 223 +1
Lines 8047 8050 +3
Branches 2208 2210 +2
=======================================
+ Hits 7965 7968 +3
Misses 28 28
Partials 54 54
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install Andarist/rollup#fix/illegal-identifier-exported-in-es or load it into the REPL: |
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.
Thanks for tackling this! While the fix is fine, there are some more import/reexport cases I think need to be covered, would you be willing to address them as well?
src/finalisers/es.ts
Outdated
@@ -131,7 +135,11 @@ function getExportBlock(exports: ChunkExports, { _, cnst }: GenerateCodeSnippets | |||
exportDeclaration.push( | |||
specifier.exported === specifier.local | |||
? specifier.local | |||
: `${specifier.local} as ${specifier.exported}` | |||
: `${specifier.local} as ${ |
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.
There should be at least two other places to check, see this example:
https://rollupjs.org/repl/?pr=4939&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QiclM0EnJTIwYXMlMjBiYXolN0QlMjBmcm9tJTIwJ2V4dGVybmFsJyUzQiU1Q25jb25zb2xlLmxvZyhiYXopJTNCJTVDbmNvbnN0JTIwZm9vJTIwJTNEJTIwJ2JhciclM0IlNUNuZXhwb3J0JTIwJTdCZm9vJTIwYXMlMjAnJTJCJyU3RCU1Q25leHBvcnQlMjAlN0JiYXIlMjBhcyUyMCctJyUyQyUyMCclMkYnJTdEJTIwZnJvbSUyMCdleHRlcm5hbCclM0IlNUNuJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlMkMlMjJuYW1lJTIyJTNBJTIybWFpbi5qcyUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJvdXRwdXQlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlcyUyMiU3RCUyQyUyMnRyZWVzaGFrZSUyMiUzQXRydWUlN0QlN0Q=
- imports
- reexports
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
.
@@ -0,0 +1,6 @@ | |||
module.exports = { |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
moved this to the requested location
I've fixed the mentioned cases and a couple more. I think that I'm wrapping almost every location that has to be wrapped with the However, I intentionally left one spot not wrapped with this. I discovered that export * as '🙈' from 'external'; So I couldn't add this to a test right now. I could wrap the required call site preemptively but I would prefer to only do it once Also, thanks to the fact that you have used math operators in your proposed test cases... I discovered that this doesn't work properly either (and I don't plan to work on fixing it rn 😉): // input
import { '*' as test } from 'external'
console.log(test)
// actual
import * as test from 'external';
console.log(test)
// expected
import { '*' as _safe } from 'external'
console.log(_safe) |
cbd94e6
to
f1538ce
Compare
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.
Looks great! One might consider using some sanitizer function instead of _safe
, but that is not really important as the deconflicting logic works as intended. I will merge this one now.
This PR has been released as part of rollup@3.20.5. You can test it via |
I believe this change breaks my builds. They run fine with 3.20.4 and break with 3.20.5. I don't know what to look for and I can't provide a code to reproduce the problem, since it's private. Maybe you have an idea anyway or can direct me how to diagnose to root cause? I'm getting I added some logging and found that MagicString.update is invoked with "_safe" and |
Might be super hard to find this if you don't provide a repro case. Perhaps try to slap an extra global counter on each |
I think I will roll back the feature until we figured out what is wrong. Nicely we know have a reproduction at #4945 |
I'm using Minimal examplerollup.config.mjsimport dts from 'rollup-plugin-dts';
export default [
{
input: './src/index.ts',
output: [{ file: `dist/index.d.ts`, format: 'es' }],
plugins: [dts()],
},
]; src/index.tsimport { Dispatcher } from 'undici';
export function makeDispatcherProvider() {
let d: Dispatcher;
return {
getDispatcher() {
return d;
},
setDispatcher(newDispatcher: Dispatcher) {
d = newDispatcher;
},
};
}
|
Great that you could reproduce this, thank you! |
This PR contains:
Are tests included?
Breaking Changes?
Description
This is legal JS and it should produce legal ES output 😉