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

Don't export $$default #6328

Merged
merged 3 commits into from
Jul 31, 2023
Merged

Don't export $$default #6328

merged 3 commits into from
Jul 31, 2023

Conversation

zth
Copy link
Collaborator

@zth zth commented Jul 16, 2023

Closes #4906 (comment).

Investigation

  • The compiler exports any invalid identifier as $$<identifierName>. For example, let else = "hello" is valid ReScript, but not valid JS because else is a keyword. Therefore, you couldn't export else from a JS file, even if it's valid ReScript. So, the compiler prepends $$ to any identifier it thinks might be invalid. And via that, it also exports the identifier as $$<identifierName>. The alternative would be to not export those identifiers at all, which doesn't really make sense.
  • However, default is a special case that might just be possible to support, because we're always adding a dedicated default export whenever we see default. So, even if $$default isn't exported anymore, we should be able to tweak the compiler to just refer to default where it now refers to $$default (done).

@zth zth mentioned this pull request Jul 16, 2023
@zth
Copy link
Collaborator Author

zth commented Jul 30, 2023

This feels low risk and I feel like merging it after it has been reviewed. Thoughts @cristianoc ?

@zth zth marked this pull request as ready for review July 30, 2023 19:44
@zth zth changed the title [exploratory] Don't export $$default Don't export $$default Jul 30, 2023
@zth zth force-pushed the prevent-export-dollardollar-default branch from 9560656 to 7fad0d1 Compare July 31, 2023 09:45
@zth zth merged commit 1615975 into master Jul 31, 2023
14 checks passed
@zth zth deleted the prevent-export-dollardollar-default branch July 31, 2023 10:26
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.

Prevent export of $$default
2 participants