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

Changes exports to ExportSpecifier. #119

Merged
merged 3 commits into from
Jul 27, 2022
Merged

Conversation

tommie
Copy link
Contributor

@tommie tommie commented Jul 26, 2022

Closes #116.

Adds information about the local name for "export { a as b }" and whether it's an ambient export or not.

Breaking change for the parse() API since exports is no longer an array of strings.

tommie added a commit to tommie/es-module-shims that referenced this pull request Jul 26, 2022
Adds information about the local name for "export { a as b }" and
whether it's an ambient export or not.

Breaking change for the parse() API since exports is no longer an
array of strings.
@guybedford
Copy link
Owner

I'm wondering if I've fully thought through the "ambient" concept. I guess the point is to try and work out when a local binding is live in that it could change. Perhaps the local binding existing at all is the only thing that affects that though I suppose? In which case any if (ambient) check could just be if (localBinding)? Apologies for introducing confusion there if I didn't fully think it through!

As for the identifier thing with default, the point here is that default is not a valid identifier therefore shouldn't be treated as "live" either. BUT again I think I've mis-assigned the issue here in that this is fully captured by localName since it will always be a valid local identifier by definition. So for both ambient and the identifier checks, I believe I'm completely wrong and localName being defined is all we need? Please double check and confirm I've got this right now?

I had a look at the CI issues. The Windows failure was a CI regression, which I've just removed for now. But the other failure seems specific to the PR. While I could get the tests running locally it may be some other kind of CI issue. Will look into it further soon. In the mean time, let me know if the above localName alignment makes sense to you. Thanks again for the work on this!

@guybedford
Copy link
Owner

Released in 1.0.0.

Comment on lines +97 to +98
* exports[0].n;
* // Returns "asdf"
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't that be an example of ln?

so more like

   * exports[0].ln;
   * // Returns "a"

?

or am I wrong in the assumption that this is the way to get "a"?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah that's definitely a typo, thanks for spotting it!

@tommie
Copy link
Contributor Author

tommie commented Jul 27, 2022

Thanks for getting this merged so quickly!

I just realized that export default function g() {} should probably set ln = "g". At least nodejs seems to make g available locally. I don't think we have any use for this in es-module-shims, and the complication of tryParseExportStatement() is probably not worth it.

Edit: and export default class A {}.

@guybedford
Copy link
Owner

Yes, you're right on these. I think we should do that, since they are actually assignable as well would you believe it. But that can be a follow-up fix anytime.

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.

More detailed export statement tracking
3 participants