Bug 167698 – Web Inspector: Upgrade Esprima to the latest one to support dynamic import

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

[Yusuke Suzuki]

Dynamic import is now implemented in Esprima trunk (https://github.com/jquery/esprima/commit/11a2385739d523c62f2989c0fdafebfd0520a977).

[Yusuke Suzuki]

Created attachment 300349 [details]
Patch

[Saam Barati]

Comment on attachment 300349 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:1028
> +        case "Import":
> +            result = {
> +                type: WebInspector.ScriptSyntaxTree.NodeType.Import
> +            };

Is this only for dynamic import? If so:
1. We should gather data on the import expression as well? 
2. Perhaps a better name for our node type would be DynamicImport or ImportCall?

[Joseph Pecoraro]

Comment on attachment 300349 [details]
Patch

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

r=me! Really well done!!

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:859
> +                argument: this._createInternalSyntaxTree(node.argument)

Nit: Add the trailing comma, it makes possible future diffs nicer.

>> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:1028
>> +            };
> 
> Is this only for dynamic import? If so:
> 1. We should gather data on the import expression as well? 
> 2. Perhaps a better name for our node type would be DynamicImport or ImportCall?

Each of the AST libraries follows the ESTree AST Spec. They decided to call this Import as of a few days ago. We should keep the name "Import" to match:
https://github.com/estree/estree/commit/8ab62755743819a524c3d9a2537f34208ce26d68
https://github.com/estree/estree/pull/137

> LayoutTests/inspector/formatting/formatting-javascript.html:21
> +        "resources/javascript-tests/import.js",

You'll want to also add "import.js" anywhere in the of tests in the formatter tool:
Source/WebInspectorUI/Tools/Formatting/index.html (around line 45)

> LayoutTests/inspector/formatting/resources/javascript-tests/object-array-literal.js:11
> +o={a:1,b:2,c:3,d:4,...e}

We should add a test for this case as well:

    o={...e}

> LayoutTests/inspector/formatting/resources/javascript-tests/variable-declaration.js:26
> +var {type,...rest} = { type: "Cocoa", taste: "Sweet" };

We should add a test for this case as well:

    var {...rest}={a:1,b:2};

And it seems crazy but I don't see any tests for array destructuring. So you might want to add a few basic one to cover that too!

    var [a,b]=[0,1];

[Yusuke Suzuki]

Comment on attachment 300349 [details]
Patch

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

Thanks!

>> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:859
>> +                argument: this._createInternalSyntaxTree(node.argument)
> 
> Nit: Add the trailing comma, it makes possible future diffs nicer.

Done.

>>> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:1028
>>> +            };
>> 
>> Is this only for dynamic import? If so:
>> 1. We should gather data on the import expression as well? 
>> 2. Perhaps a better name for our node type would be DynamicImport or ImportCall?
> 
> Each of the AST libraries follows the ESTree AST Spec. They decided to call this Import as of a few days ago. We should keep the name "Import" to match:
> https://github.com/estree/estree/commit/8ab62755743819a524c3d9a2537f34208ce26d68
> https://github.com/estree/estree/pull/137

Yeah,

1.
This `Import` node is used with CallExpression. So if you evaluate `import(...)`, this becomes CallExpression.
And the calle of this CallExpression becomes this `Import` node. Therefore, this import node does not have any argument.
This is similar to the `super(...)` node representation in estree. And this is why we use `"Import"` for this node name in Esprima.

2.
In this patch, I think we should stick on the node name used in Esprima and estree.

>> LayoutTests/inspector/formatting/formatting-javascript.html:21
>> +        "resources/javascript-tests/import.js",
> 
> You'll want to also add "import.js" anywhere in the of tests in the formatter tool:
> Source/WebInspectorUI/Tools/Formatting/index.html (around line 45)

Nice! I've added this import.js to that list too.

>> LayoutTests/inspector/formatting/resources/javascript-tests/object-array-literal.js:11
>> +o={a:1,b:2,c:3,d:4,...e}
> 
> We should add a test for this case as well:
> 
>     o={...e}

Nice catch. Added.

>> LayoutTests/inspector/formatting/resources/javascript-tests/variable-declaration.js:26
>> +var {type,...rest} = { type: "Cocoa", taste: "Sweet" };
> 
> We should add a test for this case as well:
> 
>     var {...rest}={a:1,b:2};
> 
> And it seems crazy but I don't see any tests for array destructuring. So you might want to add a few basic one to cover that too!
> 
>     var [a,b]=[0,1];

Yeah, nice. Added.

[Yusuke Suzuki]

Committed r211554: <http://trac.webkit.org/changeset/211554>

[Saam Barati]

Comment on attachment 300349 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:1028
>>>> +            };
>>> 
>>> Is this only for dynamic import? If so:
>>> 1. We should gather data on the import expression as well? 
>>> 2. Perhaps a better name for our node type would be DynamicImport or ImportCall?
>> 
>> Each of the AST libraries follows the ESTree AST Spec. They decided to call this Import as of a few days ago. We should keep the name "Import" to match:
>> https://github.com/estree/estree/commit/8ab62755743819a524c3d9a2537f34208ce26d68
>> https://github.com/estree/estree/pull/137
> 
> Yeah,
> 
> 1.
> This `Import` node is used with CallExpression. So if you evaluate `import(...)`, this becomes CallExpression.
> And the calle of this CallExpression becomes this `Import` node. Therefore, this import node does not have any argument.
> This is similar to the `super(...)` node representation in estree. And this is why we use `"Import"` for this node name in Esprima.
> 
> 2.
> In this patch, I think we should stick on the node name used in Esprima and estree.

Makes sense. Thanks for clarifying.