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

Flattening imports MUST take care of javascript reserved words #2680

Closed
matthiasg opened this issue Feb 5, 2019 · 4 comments · Fixed by #2689
Closed

Flattening imports MUST take care of javascript reserved words #2680

matthiasg opened this issue Feb 5, 2019 · 4 comments · Fixed by #2689

Comments

@matthiasg
Copy link

  • Rollup Version: v1.2.2
  • Operating System (or Browser): Windows 10
  • Node Version: 10.15.0

How Do We Reproduce?

See https://github.com/matthiasg/rollup-reserved-word-issue/

Basically importing like this import * as express from 'express' and then using express.static does not work since the call is flattened to just static which is a reserved word.

Expected Behavior

The resulting file should either leave the module.static call or rename static to a non-reserved word

Actual Behavior

index.js → stdout...
[!] Error: The keyword 'static' is reserved
module.js (1:16)
1: export function static(text) {
                   ^
2:   console.log(text)
3: }
Error: The keyword 'static' is reserved
    at error (C:\Users\mgt57\AppData\Roaming\npm-cache\_npx\12096\node_modules\rollup\dist\rollup.js:3597:30)
    at Module.error (C:\Users\mgt57\AppData\Roaming\npm-cache\_npx\12096\node_modules\rollup\dist\rollup.js:14376:9)
    at tryParse (C:\Users\mgt57\AppData\Roaming\npm-cache\_npx\12096\node_modules\rollup\dist\rollup.js:14039:16)
    at Module.setSource (C:\Users\mgt57\AppData\Roaming\npm-cache\_npx\12096\node_modules\rollup\dist\rollup.js:14095:35)
    at C:\Users\mgt57\AppData\Roaming\npm-cache\_npx\12096\node_modules\rollup\dist\rollup.js:17409:20
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at findNodeScript.then.existing (C:\Users\mgt57\AppData\Roaming\npm\node_modules\npm\node_modules\libnpx\index.js:268:14)

index.js

import * as module from './module'

module.static('test')

module.js

export function static(text) {
  console.log(text)
}
@gund
Copy link

gund commented Feb 6, 2019

I hit same bug rendering compiled bundles broken.

Here is simple example.

I'm using io-ts library inside my own library and it has exports named undefined and null.

While undefined is being renamed to undefined$1, null is kept not renamed so generated code is invalid.

NOTE: This is only a bug while emitting in esm and systemjs format.

@matthiasg
Copy link
Author

matthiasg commented Feb 7, 2019

BTW: Workaround right now is a little involved.

in my case

const staticPropertyName = 'static'
express[staticPropertyName](...)

It has to be indirected via a variable, calling express['static'] gets resolved and renaming on import as in import { static as renamedStatic } from 'express' also gets resolved and thus doesn't work.

@lukastaegert
Copy link
Member

Hi, thanks for this issue. As a matter of fact, I am currently working on a complete revamp of the variable resolution/deconflicting logic where it would be easy to incorporate these fixes. Will still take a little while, though.

gund pushed a commit to orchestratora/gen-io-ts that referenced this issue Feb 7, 2019
@lukastaegert
Copy link
Member

Possible fix a #2689

lukastaegert added a commit that referenced this issue Feb 17, 2019
* Refactor scope structure

* Initial draft for new deconflicting solution with variable access tracking

* Slightly improve some names

* Deconflict CJS interop function

* Cache findVariable via accessedOutsideVariables

* Get rid of unnecessary parameter and improve name

* Use objects instead of Sets to track used names

* Only track global outside variables on module scopes, re-unify external
variable handling with other import handling

* Move warning about missing exports into Graph

* Test variables are no longer needlessly deconflicted due to globals in other chunks

* Use tree-shaking information when deconflicting

* Deconflict build-in names, resolves #2680

* Make sure it resolves #2683

* Clean up chunk deconflicting code

* Change priorities so that chunk variable names take precedence over
imported variable names

* Fix acorn import fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants