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 arbitrary string exports #54

Merged
merged 2 commits into from
Apr 28, 2021

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Apr 23, 2021

This PR includes two changes:

  • exports.package (where package is a reserved word in strict mode) is now allowed.
    This seemed to be an arbitrary limitation, given that you can do import { package as pkg } from "dep" and that exports.var was already allowed.
  • Allow arbitrary strings when doing exports["foo bar"], rather than forcing the string contents to be an identifier. They can be imported in Node.js 16 using import { "foo bar" as foobar } from "dep", or in older Node.js versions using import * as ns from "dep"; ns["foo bar"].

I also refactored a bit how strings are parsed, by using a single stringLiteral(quote) function rather than having to branch between ' and " every time.

Thanks @bmeck for making me realize that they didn't work.

TODO:

  • Check how Babel compiles export { x as "foo bar" }, and make sure that it works. EDIT: Babel just generates exports["foo bar"] = x;.

Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems ok to me. It's a shame we only got to this now as it's likely only 14+ at this point at best, but I'd be happy to release and backport if we have approval from others as well.

@bmeck can you provide your review here as well to confirm?

@bmeck
Copy link
Member

bmeck commented Apr 27, 2021

this looks good to me. I'd actually be hesitant to port this to 14, since native won't support it.

@guybedford
Copy link
Collaborator

guybedford commented Apr 28, 2021

@bmeck although import * as m from 'cjs'; m['...'] would work for Node.js 14, so perhaps still worthwhile for that at least? I'd like to at least keep the backports going as much as possible for future changes so would prefer not to maintain two separate branches here.

@guybedford guybedford merged commit 8a107bd into nodejs:master Apr 28, 2021
@guybedford
Copy link
Collaborator

Published in 1.2.0, will start porting now.

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

Successfully merging this pull request may close these issues.

3 participants