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 import.meta.* for systemjs format #3152

Merged
merged 2 commits into from Oct 15, 2019
Merged

Conversation

dmail
Copy link
Contributor

@dmail dmail commented Oct 11, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers: #3151

Description

This PR mark module global variable as referenced for any property of import.meta, not only for import.meta.url.
I have focused on systemjs format because I don't really know what to do with other formats.

@codecov
Copy link

codecov bot commented Oct 11, 2019

Codecov Report

Merging #3152 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3152      +/-   ##
==========================================
- Coverage   90.12%   90.12%   -0.01%     
==========================================
  Files         165      165              
  Lines        5874     5873       -1     
  Branches     1790     1789       -1     
==========================================
- Hits         5294     5293       -1     
  Misses        350      350              
  Partials      230      230
Impacted Files Coverage Δ
src/ast/nodes/MetaProperty.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b16548...af544a5. Read the comment docs.

@lukastaegert
Copy link
Member

Thanks! I see your point, there probably is no "natural" choice for any format besides esm and SystemJS. I hope to have a look soon. The undefined is not really a result of your fix but have been there all along: https://rollupjs.org/repl/?version=1.23.1&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmNvbnNvbGUubG9nKGltcG9ydC5tZXRhLnJlc29sdmUoKSklM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJjanMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

The reason is this:

return prop === null ? `({ url: ${urlMechanism} })` : prop === 'url' ? urlMechanism : 'undefined';

On second thought, I believe it makes sense to do it that way. As for these formats, we do not know where to look for these properties, we assume them to be undefined. This will throw an error if they are actually accessed but will otherwise produce valid code if not.

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.

None yet

2 participants