Fixed imports/exports that are illegal identifiers in the es output #4939

Andarist · 2023-04-14T09:14:26Z

This PR contains:

bugfix

Are tests included?

yes

Breaking Changes?

no

Description

This is legal JS and it should produce legal ES output 😉

const foo = 1
export { foo as "🔥" }

vercel · 2023-04-14T09:14:29Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
rollup	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 18, 2023 4:34am

Andarist · 2023-04-14T09:15:42Z

src/finalisers/es.ts

@@ -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 ${


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 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 · 2023-04-14T09:22:48Z

Codecov Report

Merging #4939 (76fcbbc) into master (f1538ce) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           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

Impacted Files	Coverage Δ
src/finalisers/es.ts	`100.00% <100.00%> (ø)`
src/utils/generateCodeSnippets.ts	`97.14% <100.00%> (-0.16%)`	⬇️
src/utils/isValidIdentifier.ts	`100.00% <100.00%> (ø)`
src/utils/safeName.ts	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2023-04-17T04:42:43Z

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:
https://rollup-268ffu7sm-rollup-js.vercel.app/repl/?pr=4939

lukastaegert

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?

lukastaegert · 2023-04-17T04:49:38Z

src/finalisers/es.ts

@@ -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 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.

lukastaegert · 2023-04-17T04:52:51Z

test/chunking-form/samples/export-illegal-identifier/_config.js

@@ -0,0 +1,6 @@
+module.exports = {


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.

moved this to the requested location

Andarist · 2023-04-17T11:04:01Z

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?

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 safeExportName in the es finalizer. There are locations in there that might look like then need wrapping at the first glance but I think that they refer to local bindings and those always have to be valid so I left them untouched (I'm only trying to wrap what has to be wrapped and I try to avoid doing redundant "work" where it's not needed).

However, I intentionally left one spot not wrapped with this. I discovered that acorn+acorn-import-assertions fails to parse this:

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 acorn-import-assertions fixes the issue.

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)

lukastaegert

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.

rollup-bot · 2023-04-18T05:04:55Z

This PR has been released as part of rollup@3.20.5. You can test it via npm install rollup.

JochenDiekenbrock · 2023-04-18T09:18:02Z

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
[!] Error: Cannot overwrite a zero-length range – use appendLeft or prependRight instead
at MagicString.update (/home/jochen/WebstormProjects/shared/node_modules/rollup/dist/shared/rollup.js:2142:10)

I added some logging and found that MagicString.update is invoked with "_safe" and
that the function getSafeName finds a lot of invalid identifiers. The last call to getSafeName before the error above is with
baseName 2038 and the generated safeName is "_safe"

Andarist · 2023-04-18T09:23:13Z

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 _safe and then check which one ends up crashing MagicString there. This could help you to identify which exact invalid identifier relates to this crash - this could help you to minimize the repro further.

lukastaegert · 2023-04-18T11:16:29Z

I think I will roll back the feature until we figured out what is wrong. Nicely we know have a reproduction at #4945

webcarrot · 2023-04-18T11:16:33Z

@Andarist

I'm using rollup-plugin-dts and I started to have the same issue.

Minimal example

rollup.config.mjs

import dts from 'rollup-plugin-dts';
export default [
	{
		input: './src/index.ts',
		output: [{ file: `dist/index.d.ts`, format: 'es' }],
		plugins: [dts()],
	},
];

src/index.ts

import { Dispatcher } from 'undici';

export function makeDispatcherProvider() {
	let d: Dispatcher;
	return {
		getDispatcher() {
			return d;
		},
		setDispatcher(newDispatcher: Dispatcher) {
			d = newDispatcher;
		},
	};
}

`rollup -c ./rollup.config.mjs`

./src/index.ts → dist/index.d.ts...
[!] Error: Cannot overwrite a zero-length range – use appendLeft or prependRight instead
    at MagicString.update (/DIR/node_modules/rollup/dist/shared/rollup.js:2142:10)
    at MagicString.overwrite (/DIR/node_modules/rollup/dist/shared/rollup.js:2131:15)
    at Identifier.render (/DIR/node_modules/rollup/dist/shared/rollup.js:8762:22)
    at AssignmentPattern.render (/DIR/node_modules/rollup/dist/shared/rollup.js:9439:19)
    at FunctionDeclaration.render (/DIR/node_modules/rollup/dist/shared/rollup.js:6346:28)
    at renderStatementList (/DIR/node_modules/rollup/dist/shared/rollup.js:8913:35)
    at Program.render (/DIR/node_modules/rollup/dist/shared/rollup.js:12199:13)
    at Module.render (/DIR/node_modules/rollup/dist/shared/rollup.js:14105:18)
    at Chunk.renderModules (/DIR/node_modules/rollup/dist/shared/rollup.js:16554:41)
    at Chunk.render (/DIR/node_modules/rollup/dist/shared/rollup.js:16062:111)

JochenDiekenbrock · 2023-04-18T11:27:37Z

@webcarrot

I'm using rollup-plugin-dts and I started to have the same issue.

Great that you could reproduce this, thank you!
My case is almost identical to this one, just a very simple config with dts()

Fixed exports that are illegal identifiers in the es output

a56d936

vercel bot deployed to Preview April 14, 2023 09:15 View deployment

Andarist commented Apr 14, 2023

View reviewed changes

Unify shared~ regex into isValidIdentifier util

7d8a186

vercel bot deployed to Preview April 14, 2023 09:21 View deployment

Andarist mentioned this pull request Apr 14, 2023

Fixed the default export shape in strict ESM environments preconstruct/preconstruct#543

Merged

lukastaegert added the x⁸ ⚙️ build repl artefacts label Apr 17, 2023

lukastaegert reviewed Apr 17, 2023

View reviewed changes

fixed more cases

09208df

Andarist changed the title ~~Fixed exports that are illegal identifiers in the es output~~ Fixed imports/exports that are illegal identifiers in the es output Apr 17, 2023

vercel bot deployed to Preview April 17, 2023 10:29 View deployment

Andarist requested a review from lukastaegert April 17, 2023 14:53

lukastaegert force-pushed the master branch 3 times, most recently from cbd94e6 to f1538ce Compare April 17, 2023 16:58

Merge branch 'master' into fix/illegal-identifier-exported-in-es

76fcbbc

vercel bot deployed to Preview April 18, 2023 04:34 View deployment

lukastaegert approved these changes Apr 18, 2023

View reviewed changes

lukastaegert merged commit 3bbf6ae into rollup:master Apr 18, 2023
12 checks passed

Andarist mentioned this pull request Apr 18, 2023

Add a test for reserved keywords used as import/export specifiers #4943

Merged

Andarist deleted the fix/illegal-identifier-exported-in-es branch April 18, 2023 07:58

EnduIf added a commit to ShadowBr0ther/Pornhub.js that referenced this pull request Apr 18, 2023

Rollup bug: rollup/rollup#4939

2e5a807

lukastaegert added a commit that referenced this pull request Apr 18, 2023

Revert #4939 for now

4691cba

lukastaegert mentioned this pull request Apr 18, 2023

Revert #4939 for now #4946

Merged

9 tasks

lukastaegert added a commit that referenced this pull request Apr 18, 2023

Revert #4939 for now (#4946)

911700b

ShadowBr0ther pushed a commit to ShadowBr0ther/Pornhub.js that referenced this pull request Jan 25, 2024

Rollup bug: rollup/rollup#4939

0f8d0dd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed imports/exports that are illegal identifiers in the es output #4939

Fixed imports/exports that are illegal identifiers in the es output #4939

Andarist commented Apr 14, 2023

vercel bot commented Apr 14, 2023 •

edited

Andarist Apr 14, 2023

lukastaegert Apr 17, 2023

codecov bot commented Apr 14, 2023 •

edited

github-actions bot commented Apr 17, 2023 •

edited

lukastaegert left a comment

lukastaegert Apr 17, 2023

lukastaegert Apr 17, 2023

Andarist Apr 17, 2023

Andarist commented Apr 17, 2023 •

edited

lukastaegert left a comment

rollup-bot commented Apr 18, 2023

JochenDiekenbrock commented Apr 18, 2023 •

edited

Andarist commented Apr 18, 2023

lukastaegert commented Apr 18, 2023

webcarrot commented Apr 18, 2023

JochenDiekenbrock commented Apr 18, 2023

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

Andarist commented Apr 14, 2023

Description

vercel bot commented Apr 14, 2023 • edited

Andarist Apr 14, 2023

Choose a reason for hiding this comment

lukastaegert Apr 17, 2023

Choose a reason for hiding this comment

codecov bot commented Apr 14, 2023 • edited

Codecov Report

github-actions bot commented Apr 17, 2023 • edited

Thank you for your contribution! ❤️

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert Apr 17, 2023

Choose a reason for hiding this comment

lukastaegert Apr 17, 2023

Choose a reason for hiding this comment

Andarist Apr 17, 2023

Choose a reason for hiding this comment

Andarist commented Apr 17, 2023 • edited

lukastaegert left a comment

Choose a reason for hiding this comment

rollup-bot commented Apr 18, 2023

JochenDiekenbrock commented Apr 18, 2023 • edited

Andarist commented Apr 18, 2023

lukastaegert commented Apr 18, 2023

webcarrot commented Apr 18, 2023

Minimal example

rollup.config.mjs

src/index.ts

rollup -c ./rollup.config.mjs

JochenDiekenbrock commented Apr 18, 2023

vercel bot commented Apr 14, 2023 •

edited

codecov bot commented Apr 14, 2023 •

edited

github-actions bot commented Apr 17, 2023 •

edited

Andarist commented Apr 17, 2023 •

edited

JochenDiekenbrock commented Apr 18, 2023 •

edited

`rollup -c ./rollup.config.mjs`