-
-
Notifications
You must be signed in to change notification settings - Fork 258
Conversation
Codecov Report
@@ Coverage Diff @@
## master #544 +/- ##
==========================================
- Coverage 98.14% 96.82% -1.33%
==========================================
Files 22 22
Lines 3607 3617 +10
Branches 1007 1010 +3
==========================================
- Hits 3540 3502 -38
- Misses 24 58 +34
- Partials 43 57 +14
Continue to review full report at Codecov.
|
The CI failure seems to be unrelated to this change..? |
Should make sure to disallow |
Added tests to verify that:
|
const x = import.meta; | ||
const url = import.meta.url; | ||
import.meta; | ||
import.meta.url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason this test didn't also have import.meta.couldBeMutable = true;
? But the other did?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No deeper reason than "order of copy&paste". ^^ Copied the actual.js
/expected.json
over again to keep them in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! really fast 😄
cc @domenic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me (but I'm not familiar with the parser at all).
AST format is:
MetaProperty {
meta: Identifier { name: 'import' }
property: Identifier { name: 'meta' }
}
Just need to do something about the error message when parsing, say, import.notMeta
.
src/parser/expression.js
Outdated
if (this.options.sourceType !== "module") { | ||
this.raise(id.start, "import.meta can only be used in modules"); | ||
} | ||
return this.parseMetaProperty(node, id, "meta"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message if the meta property isn't .meta
is confusing (it's hardcoded to mention new
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, good point. Added a test that checks for that & made it use the correct name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
src/parser/expression.js
Outdated
const node = this.startNode(); | ||
const id = this.parseIdentifier(true); | ||
this.expect(tt.dot); | ||
if (this.options.sourceType !== "module") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can also do if (!this.inModule)
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I thought I tried that first but .inModule
was only true top-level. Must have missed something. You're right, definitely works fine.
src/parser/expression.js
Outdated
const id = this.parseIdentifier(true); | ||
this.expect(tt.dot); | ||
if (this.options.sourceType !== "module") { | ||
this.raise(id.start, "import.meta can only be used in modules"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to make this consistent with other sourceType errors?
'import.meta' can only be used with 'sourceType: module'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, good point. Copied the wording from "can only be used in functions". Should've taken the wording from import/export statements. Fixed.
This might be irrelevant, or possibly a subject to have in a new issue, but I'm curious about the drops in coverage? For example, it says above that six files were impacted, but four of them aren't changed at all:
|
@rwaldron Yeah, I was wondering about the same thing. I tried rebasing on master in case it was using some weird merge head but afaict that wasn't it. |
Fixes #539
Relatively straight-forward: Parses
import.meta
as a newMetaProperty
. The expression is valid in modules but not in scripts. It can be enabled even whendynamicImport
isn't enabled.