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

[FIX] Switch from esprima to espree #615

Merged
merged 4 commits into from
Jun 16, 2021
Merged

Conversation

codeworrior
Copy link
Member

This is work in progress and can't be submitted as the used espree
version (8.0.0-beta) is not a release version and it requires a newer
NodeJS min version than the tooling.

espree 8.0.0-beta was chosen as it switches the default for the
ecmaVersion option from 5 (7.3.1) to latest.

Before submitting, it also should be decided whether all parsing code
should be routed through some helper utility, to simplify future updates
/ changes of the parser. At the same time, we should decide about the
use of the sourceType and the ecmaVersion options. Should we expose them
in ui5.yaml? Or should we rather leave that to the linting step?

Fixes SAP/ui5-tooling#354 .

Thank you for your contribution! 🙌

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

@codeworrior codeworrior force-pushed the switch-from-esprima-to-espree branch from 9c88240 to e7bef3b Compare June 13, 2021 07:21
@coveralls
Copy link

coveralls commented Jun 13, 2021

Coverage Status

Coverage increased (+0.005%) to 94.726% when pulling 7f117d6 on switch-from-esprima-to-espree into b86f0fa on master.

@RandomByte
Copy link
Member

At the same time, we should decide about the use of the sourceType and the ecmaVersion options. Should we expose them in ui5.yaml? Or should we rather leave that to the linting step?

As discussed earlier today, we should always parse the latest ECMAScript version (just like browsers do). Projects should use linters to enforce specific versions.
We should always assume sourceType "script" for now. In the future we might detect modules based on file suffix or package.json configuration.

To make this part of a non-major UI5 Tooling release we would need to use espree v6 as all later versions require Node.js >10.

@codeworrior
Copy link
Member Author

Going with espree ^6.0.0 however, we would miss at least the following syntax features:

  • 75e80bc Update: support ?? operator, import.meta, and export * as ns
  • 872645c Update: support optional chaining
  • bd0a405 Update: support logical assignment and numeric separators

where the second one hurts most.

@RandomByte
Copy link
Member

RandomByte commented Jun 15, 2021

Still better than nothing, right? So maybe go with v6 for now and upgrade to v8 with UI5 Tooling 3.0?

A new module lib/lbt/utils/parseJS is introduced that exposes a parseJS
function and a Syntax object with all AST node types. The new module is
used in all places where JavaSCript code is parsed (incl. tests).

The module uses espree v6 for parsing instead of esprima. This is the
maximum version that can be used without introducing breaking changes to
the tooling (higher versions of espree require higher node versions than
documented in our package.json).

The set of allowed parser options is limited to ease a later migration
to another parser.

Fixes SAP/ui5-tooling#354 .
@codeworrior codeworrior force-pushed the switch-from-esprima-to-espree branch from e7bef3b to 68557a0 Compare June 15, 2021 16:44
@RandomByte
Copy link
Member

Validated 7ea1a69 successfully in a smoke test run and locally by building the webc libraries 👍

@codeworrior codeworrior changed the title [FIX][WIP] Switch from esprima to espree [FIX] Switch from esprima to espree Jun 16, 2021
@codeworrior
Copy link
Member Author

codeworrior commented Jun 16, 2021

Are you okay with the parseJS module? In the tests, I found it a bit strange that the module has the same name as one of its named exports. If someone has a better proposal, I would be glad to rename it.

  • Module: parseUtil
  • Exports: parseJS, Syntax

@RandomByte
Copy link
Member

Yeah, parseUtil seems to be a better name for the module

@RandomByte
Copy link
Member

I'll do the rename

@RandomByte
Copy link
Member

Decided on parseUtils to align it with the already existing ASTUtils.

What about parseJs instead of parseJS 🤔 🙈

@codeworrior
Copy link
Member Author

Reg. parseJs you do what you have to, but for me it looks wrong...

@RandomByte
Copy link
Member

Well there is already JSTokenizer.parseJS, so let's rather stick with it here too.

Anything left here?

@codeworrior
Copy link
Member Author

I think we're good to go...

@RandomByte RandomByte merged commit c3d50e3 into master Jun 16, 2021
@RandomByte RandomByte deleted the switch-from-esprima-to-espree branch June 16, 2021 14:25
@pubmikeb
Copy link

Going with espree ^6.0.0 however, we would miss at least the following syntax features:

  • 75e80bc Update: support ?? operator, import.meta, and export * as ns
  • 872645c Update: support optional chaining
  • bd0a405 Update: support logical assignment and numeric separators

where the second one hurts most.

Meanwhile, there is already Espree 9.0 and Node.js 17.0, perhaps it is better to not stick with already outdated versions but go forward directly to the latest available versions, perhaps even towards late beta/RC versions. This will ensure the support of cutting edge features.

@RandomByte
Copy link
Member

According to the package.json, Espree 9.0 still supports all active Node.js versions. So it shouldn't be a problem to upgrade directly to 9.0 with our next major.

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.

"Unexpected token" error - Drop Esprima dependency?
5 participants