Skip to content
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

Allow named imports with names Storage, Plugin and more. #92

Open
prateekbh opened this issue Oct 18, 2018 · 4 comments
Open

Allow named imports with names Storage, Plugin and more. #92

prateekbh opened this issue Oct 18, 2018 · 4 comments

Comments

@prateekbh
Copy link
Member

Closure throws an error if it sees a class named Storage or Plugin.

e.g.: https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%2540compilation_level%2520SIMPLE_OPTIMIZATIONS%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250Aclass%2520Plugin%257B%257D

This might be fine as one can argue that it is a reserved word. But if these classes are a part of my imports and not global definitions, it should be a clear indication that we can safely mangle this.

What's the issue?

Closure throws error.

How do we reproduce the issue?

file-A:

export class Plugin {
}

file-B:

import {Plugin} from './file-A`
const p = new Plugin();

Run ☝️ this, with rollup-plugin-closure-compiler.

@prateekbh
Copy link
Member Author

On another thought:

May be we can expose a config where user can just specify what other names do they want to be mangled before CC looks at the code

@filipesilva
Copy link

I also see this with names like Console and Location, that the Angular libraries declare classes for. This sounds like the kind of thing assume_function_wrapper (which is set by default with this plugin) should account for.

@kristoferbaxter
Copy link
Contributor

google/closure-compiler#3098

Looks like the Closure folks are investigating a flag that would remedy this issue for the rollup plugin.

I'd prefer to use the flag instead of manual mangling like attempted in #105.

@filipesilva
Copy link

WRT mangling, @rollup/plugin-replace can be used to rename the problematic class names. It's not a real generic solution though.

crazygolem added a commit to crazygolem/markdown-muncher that referenced this issue Aug 7, 2021
The closure compiler has been replaced by terser for the minification step, as
closure does not handle correctly variables (or in this case constants)
declared with the same name as some globals (in this case `document`). This is
a valid error for scripts, but in ES modules this kind of redeclarations do not
cause problems (at least for `document`).

For details see:
- ampproject/rollup-plugin-closure-compiler#92
- google/closure-compiler#3098
- google/closure-compiler#2471
crazygolem added a commit to crazygolem/markdown-muncher that referenced this issue Aug 7, 2021
The closure compiler has been replaced by terser for the minification step, as
closure does not handle correctly variables (or in this case constants)
declared with the same name as some globals (in this case `document`). This is
a valid error for scripts, but in ES modules this kind of redeclarations do not
cause problems (at least for `document`).

For details see:
- ampproject/rollup-plugin-closure-compiler#92
- google/closure-compiler#3098
- google/closure-compiler#2471

Also terser is way faster than closure, and produces a slightly smaller
minified script.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants