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

refactor: use espree instead of esprima for parsing the code as an AST #47

Merged
merged 3 commits into from
Oct 21, 2019

Conversation

zachnthebox
Copy link
Contributor

espree is an Esprima-compatible JavaScript parser.
It looks to be drop in replacement for the work being done in this addon.

This addon prevented our team from transitioning to native classes in Ember Octane because of the dependency on esprima.

@zachnthebox zachnthebox marked this pull request as ready for review September 18, 2019 15:38
@ffaubry
Copy link
Member

ffaubry commented Oct 21, 2019

Our project is entirely Octane and we are using this Addon as it.
Could you be more specific about the issue you found?

@ggayowsky
Copy link
Contributor

While I am not the creator of this PR, I can say that our project needed to fork this repo and use espree as opposed to esprima because there was a dependency that made use of some valid ES8 syntax that esprima could not parse without throwing an error (this problem I believe jquery/esprima#1927).

Since esprima seems to be effectively dead, having this add-on depend on esprima instead of espree, can prevent an app from including some libraries.

@ffaubry
Copy link
Member

ffaubry commented Oct 21, 2019

IMO, this is not related to Octane. ArcGIS Dashboard has been ported to Octane and Esri JSAPI 4.x.
We had no issue. We are using native classes and decorators.

I think @ggayowsky is pointing in the right direction. It's the use of the stage 2/3/4 features of JS that could cause an issue as esprima is very conservative.
We could argue that these stage 2/3/4 features are not supposed to be used in production but things became greyish and devs started to use them, even VS Code is exposing them. Where is the limit? We will address these requests on a cost/benefit way.

I'm putting this out there for future reference.

As for this PR, I will go ahead and merge it. Thx to @zachnthebox for your help.

@ffaubry ffaubry merged commit b3c9291 into Esri:master Oct 21, 2019
@ggayowsky
Copy link
Contributor

ggayowsky commented Oct 21, 2019

@ffaubry Problem is that exprima doesn't support features that are in the ECMA spec (it isn't even just stage 2/3/4 features anymore), which is the real problem here.

See the issue I linked in my above comment

@ffaubry
Copy link
Member

ffaubry commented Oct 21, 2019

@ggayowsky Woah.

@zachnthebox zachnthebox deleted the zach-espree branch October 21, 2019 17:33
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.

3 participants