Implement Export{Default,Namespace}Specifier parsing ourselves. #146
Conversation
lib/parsers/acorn.js
Outdated
| @@ -1,6 +1,7 @@ | |||
| "use strict"; | |||
|
|
|||
| const acorn = require("acorn"); | |||
| const fixParser = require("./acorn-extensions.js").fix; | |||
There was a problem hiding this comment.
Maybe a more descriptive name than fix. It looks like the acorn-extensions could be a place to host more than 1 extension.
lib/parsers/acorn-extensions.js
Outdated
| while (n-- > 0) this.nextToken(); | ||
| const copy = Object.assign(Object.create(null), this); | ||
| Object.assign(this, old); | ||
| return copy; |
There was a problem hiding this comment.
We're taking a snapshot of the parser so that we can roll it back after advancing its state to look at later tokens.
There was a problem hiding this comment.
It's a shallow snapshot so not too bad.
lib/parsers/acorn-extensions.js
Outdated
| // check for keywords used as local names | ||
| for (let i = 0; i < node.specifiers.length; i++) { | ||
| const local = node.specifiers[i].local; | ||
| if (local && (this.keywords.test(local.name) || |
There was a problem hiding this comment.
what's the possible values of local? If it's either an object or undefined we should be explicit about the object check with typeof local === "object" && local !== null as v8 (and I guess others) favor that over a bool check.
There was a problem hiding this comment.
I'll do you one better: this whole loop can be eliminated (soon)!
lib/parsers/acorn-extensions.js
Outdated
|
|
||
| if (expect) { | ||
| this.unexpected(); | ||
| } else { |
lib/parsers/acorn-extensions.js
Outdated
| } | ||
|
|
||
| function checkExport(exports, name, pos) { | ||
| if (!exports) return; |
There was a problem hiding this comment.
Similar object check for exports.
| specifier.exported = this.parseIdent(true); | ||
| node.specifiers.push( | ||
| this.finishNode(specifier, "ExportNamespaceSpecifier") | ||
| ); |
There was a problem hiding this comment.
Just curious does the execution of the this.expect stuff just above matter?
I noticed one was before the const specifier =.....
There was a problem hiding this comment.
Yes, this.expect(...) modifies this.start (by calling this.eat(...) which calls this.next()), which is used by this.startNode to set the .start location of the new node, so the relative ordering of this.expect(...) and this.startNode() is important.
|
This is mostly all magic to me 😸 |
The only point of this method besides updating the map of exported symbol names was to complain if there were duplicate export names, but we override the raiseRecoverable method with noopRaiseRecoverable anyway, so there's really no need to override checkExport.
The Reify parser is more like the "loose" Acorn parser in that it doesn't care about enforcing expectations as long they don't affect the results of the parsing. So a loop that serves only to call this.unexpected is a loop that we can do without.
|
Thanks for the feedback! I think you're gonna like the follow-up commits I just pushed. |
| } | ||
|
|
||
| function peekNextType(parser) { | ||
| return parser.withLookAhead(1, () => parser.type); |
There was a problem hiding this comment.
I really like this new interface, personally.
lib/parsers/acorn.js
Outdated
| fixParser(parser); | ||
|
|
||
| acornExtensions.enableAll(parser); | ||
|
|
||
| return parser.parse(); |
There was a problem hiding this comment.
What's the lazy loading of acorn and acorn-extensions do?
There was a problem hiding this comment.
Hmm, probably not much if we always use the parser when we import this module (which I think we do). Are you concerned it might hurt?
lib/parsers/acorn-extensions.js
Outdated
| return copy; | ||
| try { | ||
| return callback.call(this); | ||
| } finally { |
There was a problem hiding this comment.
What's a typical callback look like?
If it's just () => parser.type you can optimize the callback call without the this binding and just do
return callback();
Or simplify it further.
There was a problem hiding this comment.
Ah I see isCommaOrFrom uses this. No worries.
There was a problem hiding this comment.
There's another case (isCommaOrFrom) where we use this.
Any preference between these two options?
return callback.call(this);
// or
return callback(this);Either of those could work for isCommaOrFrom.
There was a problem hiding this comment.
Yes! In event loops it's always sad to see folks default to applying a this binding because many times it's not used and does add a cost to invocation. So if you could pass in the this as the first arg that is super nice. It's what I would recommend for event systems too!
|
👍 |
Related: acornjs/acorn#541