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

Fixes rewrite for es6 code #166

Merged
merged 1 commit into from
Sep 29, 2015
Merged

Conversation

akiellor
Copy link
Contributor

It looks like this issue stemmed from us using different AST parsers for different features:

  • Rewrite is using falafel, whose default is Acorn
  • Esprima is used everywhere else

This PR migrates away from Acorn for rewrite by providing an esprima parser.

This PR creates an internal interface for parsing/walking which centralises control of which parser implementation we use and how we walk the AST.

@akiellor
Copy link
Contributor Author

@jimfleming Any thoughts on this one?

@brettlangdon
Copy link
Contributor

@akiellor do these updates cause any changes to the style guide?

@akiellor
Copy link
Contributor Author

@brettlangdon I reran the grunt fmt task and no changes were present...so I don't think so.

@brettlangdon
Copy link
Contributor

Cool. I'll see if I can take a deeper look a little later today, but from a glance this looks good.

return falafel(js, {
parser: this
}, function(node) {
node.start = node.range[0];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This is to make the esprima tree compatible with falafel's update fn's. Not sure if this sort of thing should be upstreamed to falafel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably alright to leave here, but if you could add a comment stating that fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@brettlangdon
Copy link
Contributor

I ran locally and had some changes to the style guide. Can you please run grunt fmt again?

This is the diff I got from it:

diff --git a/examples/styleGuide.js b/examples/styleGuide.js
index 5a10605..b6cb6f6 100644
--- a/examples/styleGuide.js
+++ b/examples/styleGuide.js
@@ -88,8 +88,7 @@
     myVar.toString() +
     '</li>');

-  var myVar = new myVarTypes[
-    'A type']();
+  var myVar = new myVarTypes['A type']();

   callFunc(
     'An arg',

Which I am content with this change.

@brettlangdon
Copy link
Contributor

Great, I think the only other request I have is. Do you mind squashing your commits down to a single commit? There are 10 here already for this change, squashing to one will make it easier to digest next time we dig through history :)

* Upgrade falafel
* Migrate to single method of ast parsing/walking
@akiellor
Copy link
Contributor Author

done

@brettlangdon
Copy link
Contributor

Cool, will merge right now. However I do not have push rights to npm. I will open an issue for @jimfleming to bump version and push.

brettlangdon added a commit that referenced this pull request Sep 29, 2015
@brettlangdon brettlangdon merged commit 59139b3 into rdio:master Sep 29, 2015
@akiellor
Copy link
Contributor Author

awesome...my team has been looking forward for this for a while now :)

Thanks for the quick responses!

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