Bug 200796 – Web Inspector: Update Esprima to support modern JavaScript language features

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Update Esprima to support modern JavaScript language features

Going to use a fork which implements some modern features:

  ES2018 Feature: Async Iteration (for-await-of)
  https://github.com/jquery/esprima/issues/1990

  ES2019 Feature: Numeric Separator
  https://github.com/jquery/esprima/issues/1989

  ES2019 Feature: Optional catch binding
  https://github.com/jquery/esprima/issues/1953

  ES2020 Feature: BigInt
  https://github.com/jquery/esprima/issues/1988

While going through the process to upstream support.

---

Changes to the AST are:

  * CatchClause `param` property is now nullable
  * ForOfStatement now a boolean `await` property
  * Literal can be a `"bigint"` type (works if the environment has BigInt or not)

---

The only change we need to make to our pretty-printer is add a space after `await` in for-await-of permutations.

So this:

    for await(x of []) {}

Pretty prints to:

    for await (x of []) {}

[Joseph Pecoraro]

Created attachment 376442 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

Attachment 376442 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/ChangeLog:25:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WebInspectorUI/ChangeLog:26:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WebInspectorUI/ChangeLog:27:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ross Kirsling]

Comment on attachment 376442 [details]
[PATCH] Proposed Fix

Awesome!

(Next stop, nullish-aware operators! 😁)

[Ross Kirsling]

Comment on attachment 376442 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=376442&action=review

> Source/WebInspectorUI/ChangeLog:26
> +          * ForOfStatement now a boolean `await` property

Also, I'm not really sure why the style checker is yelling at you in this area, but I do notice that you're missing a "has". :p

[Devin Rousso]

Comment on attachment 376442 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=376442&action=review

> Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:637
> +        nextJSXToken() {

Is this JSX the React JSX <https://reactjs.org/docs/introducing-jsx.html>?  If so, it'd be awesome to strip this out =D

> Source/WebInspectorUI/UserInterface/Test/TestHarness.js:204
> +    passOrFail(condition, message)

How is this different than `expectThat` or `expectTrue`?  Is it simply that you don't want to have the "Expected" and "Actual"?  If so, I'd rather change `expectThat` to be as you suggest (`expectTrue` should be the one that compares the condition to `true`, whereas `expectThat` could just be a condition).

[Joseph Pecoraro]

(In reply to Ross Kirsling from comment #4)
> Comment on attachment 376442 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376442&action=review
> 
> > Source/WebInspectorUI/ChangeLog:26
> > +          * ForOfStatement now a boolean `await` property
> 
> Also, I'm not really sure why the style checker is yelling at you in this
> area, but I do notice that you're missing a "has". :p

It doesn't like anyone using '*' for lists. I shall fix the typo!

[Joseph Pecoraro]

Comment on attachment 376442 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=376442&action=review

>> Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:637
>> +        nextJSXToken() {
> 
> Is this JSX the React JSX <https://reactjs.org/docs/introducing-jsx.html>?  If so, it'd be awesome to strip this out =D

Yes. Esprima supports JSX syntax. We can remove it at compilation time by removing a few lines. I'll file a follow-up.

Maybe we want to eventually include it?

>> Source/WebInspectorUI/UserInterface/Test/TestHarness.js:204
>> +    passOrFail(condition, message)
> 
> How is this different than `expectThat` or `expectTrue`?  Is it simply that you don't want to have the "Expected" and "Actual"?  If so, I'd rather change `expectThat` to be as you suggest (`expectTrue` should be the one that compares the condition to `true`, whereas `expectThat` could just be a condition).

Unless I'm including a "Should ..." message, then the additional output from these expect functions is very annoying. 

InspectorTest.expectThat(true, "message");
InspectorTest.expectThat(false, "message");

> PASS: message
> FAIL: message
>     Expected: truthy
>     Actual: false


InspectorTest.passOrFail(true, "message");
InspectorTest.passOrFail(false, "message");

> PASS: message
> FAIL: message

[Joseph Pecoraro]

Created attachment 376454 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 376454 [details]
[PATCH] For Landing

Clearing flags on attachment: 376454

Committed r248760: <https://trac.webkit.org/changeset/248760>

[Radar WebKit Bug Importer]

<rdar://problem/54380699>