Skip to content
This repository has been archived by the owner on Dec 9, 2022. It is now read-only.

Bump acorn version #12

Merged
merged 2 commits into from Dec 1, 2015
Merged

Bump acorn version #12

merged 2 commits into from Dec 1, 2015

Conversation

bgrins
Copy link
Member

@bgrins bgrins commented Nov 21, 2015

Changes for version compatibility:

  • Uses tokenizer instead of tokenize
  • token.type.type is renamed to token.type.label
  • token.startLoc -> token.start.loc and token.endLoc -> token.end.loc
  • The order that comments come through is different now (the onComment handler is fired immediately after the token that the comment comes after), so we don't need to use the commentQueue object. However this meant that we didn't know if we needed to add a newline after a token because we didn't know if there was going to be an inline comment next. My solution is to do one pass through the tokenizer and onComment handler and build up an array of tokens+comments, and then loop through that array and do processing. Couldn't think of an easy way to do it without storing all of the tokens in this array - I considered trying to wait and add a newline at the beginning of the loop for the last token, but I was having bugs. I'd say the current implementation is easier to follow, but I could try to go back to the single pass through if this is a performance issues (do we have performance measurements somewhere?).

Anyway, the test is passing locally and I need to test this on m-c (which I'm assuming will simultaneously require a tern update since they share the acorn implementation), but I figure it's worth starting a review here, so r? @fitzgen

@fitzgen
Copy link
Contributor

fitzgen commented Nov 23, 2015

FYI: I'm on vacation all next week, won't be able to get to this until after that.

@fitzgen
Copy link
Contributor

fitzgen commented Dec 1, 2015

Thanks for this, Brian. LGTM!

fitzgen added a commit that referenced this pull request Dec 1, 2015
@fitzgen fitzgen merged commit 35b5857 into mozilla:master Dec 1, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants