Bug 147876 – [ES6] prototyping module loader in JSC shell

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Let's implement the synchronous module loader in JSC shell side to test the dependency analysis.

[Yusuke Suzuki]

After roughly seeing the module loader draft[1], I think the details are already stated.
Since JSC already has the Promises and the microtasks system inside the framework, we can implement the module's asynchronous loader (of course, file loading is done synchronously in our first implementation).

[1]: https://whatwg.github.io/loader/

[Yusuke Suzuki]

I'm now planning to construct the loader with the ES6 Promises.
But when using it, we should do it carefully; Do not use the user-modifiable values and do not perform the user-observable operations.

Now, All the operation needed in the Loader implementation are,

1. Promise#then
2. Promise.all

So, I'll extract the non-user-observable part to the Promise operation and create the new methods, @then and @all to use them.

[Yusuke Suzuki]

Created attachment 259060 [details]
Patch

WIP

[Yusuke Suzuki]

Opened the issues related to this.

https://github.com/whatwg/loader/pull/66
https://github.com/whatwg/loader/pull/67

[Yusuke Suzuki]

Naive loader implementation based on the spec causes infinite recursion when there's circular dependencies.
I think we can solve this by introducing the one more promise that represents "this modules now resolving the dependent ones".

[Yusuke Suzuki]

Created attachment 259085 [details]
Patch

WIP

[Yusuke Suzuki]

https://github.com/whatwg/loader/issues/68 Proposed fix.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.h:151:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/Completion.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 35 files


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

[Yusuke Suzuki]

Created attachment 259087 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.h:151:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 38 files


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

[Yusuke Suzuki]

Created attachment 259100 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.h:151:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 38 files


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

[Yusuke Suzuki]

Created attachment 259101 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.h:151:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 38 files


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

[Yusuke Suzuki]

ok, pstch is ready.

[Saam Barati]

(In reply to comment #15)
> ok, pstch is ready.
Okay. I can review tomorrow night if you want.

[Yusuke Suzuki]

(In reply to comment #16)
> (In reply to comment #15)
> > ok, pstch is ready.
> Okay. I can review tomorrow night if you want.

Great! Thank you so much :D

[Yusuke Suzuki]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > ok, pstch is ready.
> > Okay. I can review tomorrow night if you want.
> 
> Great! Thank you so much :D

During your review, I'll start https://bugs.webkit.org/show_bug.cgi?id=148053,
this is partially independent work.

[Saam Barati]

Comment on attachment 259101 [details]
Patch

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

Patch is looking good.
I'd split out the promise work into another patch and make some of my suggested changes and I'll review again.
I like the implementation direction here. I think writing the Loader in JS is nice.

> Source/JavaScriptCore/ChangeLog:27
> +        To test the current module progress easily, we add the `-m` option to the JSC shell.

I think we should use `-module` instead of `-m`.

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:32
> +    if (entry.state < newState)

Will this ever be false?

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:54
> +    // 3. Instantiate

I think it would be good for us to propose to the spec better names for these phases.
Right now, we should follow the names that the spec uses, but with the hope of improving them.
I think this phase would be better named "Analyze" or "AnalayzeModule".

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:62
> +    // 4. Collect (not in the draft) https://github.com/whatwg/loader/issues/68

This phase might also have a better name, but I'm not quite sure what a proper name would be.
This phase sort of does recursive graph resolution.

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:71
> +    //     Linking means the importing and exporting the bindings with each other.

This sentence confuses me. I'm not sure what you're trying to say.

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:98
> +        module: undefined,

I think a comment saying this is a JSModuleRecord would be helpful. Maybe just "// JSModuleRecord"

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:175
> +    var instance = this.instantiation(optionalInstance, source);

maybe we can call this var moduleRecord?

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:184
> +    var deps = [];

I think this and all other uses of "deps" or other abbreviations for "dependencies"/"dependency" should spell out the full word.
So here, I think this should be "var dependencies" and the next line should be "dependenciesMap".

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:206
> +    // FIXME: Current implementation does not support optionalInstance.

I think we should have bugs for some of these FIXMEs.
Or maybe a few bugs that different FIXMEs refer to.

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:232
> +    var fetchPromise = this.fetch(key).@then(function (payload) {

Nit: This and some other function expressions don't need a space between "function" and "(".

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:285
> +        return loader.instantiate(key, source).@then(function (optionalInstance) {

Maybe it would be nice to document all hook points? I know this is one.
I know you have them listed in the module loader object, but having them in this builtin would be helpful.

> Source/JavaScriptCore/builtins/Operations.Promise.js:222
> +function promiseThen(promise, capability, onFulfilled, onRejected)

I think all the changes to the promises should be implemented in a separate patch that blocks this patch.

> Source/JavaScriptCore/runtime/Completion.cpp:136
> +    // FIXME: Now, we don't implement the linking phase yet.

I think a bug for this would be good because we really want to execute the "Ready" state of the module here in our final implementation of this function.

> Source/JavaScriptCore/runtime/ModuleLoaderObject.cpp:234
> +    return JSValue::encode(moduleAnalyzer.analyze(*moduleProgramNode));

Nit: Maybe this is silly, but can we add a line here so we see that moduleAnalyze.analyze returns a JSModuleRecord and that's what the JSValue really is.

> Source/JavaScriptCore/runtime/ModuleLoaderObject.cpp:240
> +    if (!moduleRecord)

Is this the right thing to do here? Or should we throw an error?
Same question with checks in other hooks.

> Source/JavaScriptCore/runtime/ModuleLoaderObject.h:40
> +    enum Status {

Since the actual int value here is important, maybe it's good to say something regarding the order of these.
Or list their state numbers so that it's implied you go from 1=>2=>3, etc.

[Yusuke Suzuki]

Comment on attachment 259101 [details]
Patch

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

Thank you for your review! I've spawned the Promise related one here https://bugs.webkit.org/show_bug.cgi?id=148118
And I'll reflect your comments :D

>> Source/JavaScriptCore/ChangeLog:27
>> +        To test the current module progress easily, we add the `-m` option to the JSC shell.
> 
> I think we should use `-module` instead of `-m`.

I'll change it!

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:32
>> +    if (entry.state < newState)
> 
> Will this ever be false?

Some Reflect.Loader APIs can make the situation where `entry.state < newState` becomes false.
Currently they are not implemented in this patch, it will be done.

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:54
>> +    // 3. Instantiate
> 
> I think it would be good for us to propose to the spec better names for these phases.
> Right now, we should follow the names that the spec uses, but with the hope of improving them.
> I think this phase would be better named "Analyze" or "AnalayzeModule".

Agreed. `AnalyzeModule` seems preferable.

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:62
>> +    // 4. Collect (not in the draft) https://github.com/whatwg/loader/issues/68
> 
> This phase might also have a better name, but I'm not quite sure what a proper name would be.
> This phase sort of does recursive graph resolution.

I'll change it to `ResolveDependencies` in the mean time.

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:71
>> +    //     Linking means the importing and exporting the bindings with each other.
> 
> This sentence confuses me. I'm not sure what you're trying to say.

Ah, I'll recast it,
`"Linking means that the module imports and exports the bindings from/to the other modules"`

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:98
>> +        module: undefined,
> 
> I think a comment saying this is a JSModuleRecord would be helpful. Maybe just "// JSModuleRecord"

Looks nice! I'll add this comment.

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:175
>> +    var instance = this.instantiation(optionalInstance, source);
> 
> maybe we can call this var moduleRecord?

Yes. `moduleRecord` name looks better than `instance`. I'll change it.

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:184
>> +    var deps = [];
> 
> I think this and all other uses of "deps" or other abbreviations for "dependencies"/"dependency" should spell out the full word.
> So here, I think this should be "var dependencies" and the next line should be "dependenciesMap".

Looks nice. I'll use the full names. Actually, the spec uses "deps" etc. but, "dependencies" looks clearer.

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:206
>> +    // FIXME: Current implementation does not support optionalInstance.
> 
> I think we should have bugs for some of these FIXMEs.
> Or maybe a few bugs that different FIXMEs refer to.

I'll add the bugs for each FIXME except for the spec issue.

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:232
>> +    var fetchPromise = this.fetch(key).@then(function (payload) {
> 
> Nit: This and some other function expressions don't need a space between "function" and "(".

Ah, is it better to drop this space? I'll follow it.

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:285
>> +        return loader.instantiate(key, source).@then(function (optionalInstance) {
> 
> Maybe it would be nice to document all hook points? I know this is one.
> I know you have them listed in the module loader object, but having them in this builtin would be helpful.

Sounds nice. I'll summarize the hook points in the builtins' side.

>> Source/JavaScriptCore/builtins/Operations.Promise.js:222
>> +function promiseThen(promise, capability, onFulfilled, onRejected)
> 
> I think all the changes to the promises should be implemented in a separate patch that blocks this patch.

Yeah! I've spawned it to https://bugs.webkit.org/show_bug.cgi?id=148118.

>> Source/JavaScriptCore/runtime/Completion.cpp:136
>> +    // FIXME: Now, we don't implement the linking phase yet.
> 
> I think a bug for this would be good because we really want to execute the "Ready" state of the module here in our final implementation of this function.

Yup. I'll file it and add the comment here.

>> Source/JavaScriptCore/runtime/ModuleLoaderObject.cpp:234
>> +    return JSValue::encode(moduleAnalyzer.analyze(*moduleProgramNode));
> 
> Nit: Maybe this is silly, but can we add a line here so we see that moduleAnalyze.analyze returns a JSModuleRecord and that's what the JSValue really is.

Looks nice. Fixed.

>> Source/JavaScriptCore/runtime/ModuleLoaderObject.cpp:240
>> +    if (!moduleRecord)
> 
> Is this the right thing to do here? Or should we throw an error?
> Same question with checks in other hooks.

This is a little bit conservative guard. Since these methods only called from the builtin internal ModuleLoader object.
So I can replace them with the assertion. I'll do so.

>> Source/JavaScriptCore/runtime/ModuleLoaderObject.h:40
>> +    enum Status {
> 
> Since the actual int value here is important, maybe it's good to say something regarding the order of these.
> Or list their state numbers so that it's implied you go from 1=>2=>3, etc.

Make sense, I'll add the actual values.

[Saam Barati]

Comment on attachment 259101 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:27
>>> +        To test the current module progress easily, we add the `-m` option to the JSC shell.
>> 
>> I think we should use `-module` instead of `-m`.
> 
> I'll change it!

I might be wrong about this. It looks like there is precedent for the single letter options. 
Up to you.

[Yusuke Suzuki]

Comment on attachment 259101 [details]
Patch

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

>>>> Source/JavaScriptCore/ChangeLog:27
>>>> +        To test the current module progress easily, we add the `-m` option to the JSC shell.
>>> 
>>> I think we should use `-module` instead of `-m`.
>> 
>> I'll change it!
> 
> I might be wrong about this. It looks like there is precedent for the single letter options. 
> Up to you.

oh, ok, so, just use `-m`, because the other options also use the single letter.

[Saam Barati]

Comment on attachment 259101 [details]
Patch

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

>>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:232
>>> +    var fetchPromise = this.fetch(key).@then(function (payload) {
>> 
>> Nit: This and some other function expressions don't need a space between "function" and "(".
> 
> Ah, is it better to drop this space? I'll follow it.

Actually, I'm not 100% sure about this. I know this is Web Inspector style,
but maybe JSC uses another style for built ins. It's probably best to follow
JSC's style.

[Yusuke Suzuki]

Comment on attachment 259101 [details]
Patch

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

>>>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:232
>>>> +    var fetchPromise = this.fetch(key).@then(function (payload) {
>>> 
>>> Nit: This and some other function expressions don't need a space between "function" and "(".
>> 
>> Ah, is it better to drop this space? I'll follow it.
> 
> Actually, I'm not 100% sure about this. I know this is Web Inspector style,
> but maybe JSC uses another style for built ins. It's probably best to follow
> JSC's style.

I see. So, I'll align to the builtin's code.

[Yusuke Suzuki]

Comment on attachment 259101 [details]
Patch

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

>>>>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:232
>>>>> +    var fetchPromise = this.fetch(key).@then(function (payload) {
>>>> 
>>>> Nit: This and some other function expressions don't need a space between "function" and "(".
>>> 
>>> Ah, is it better to drop this space? I'll follow it.
>> 
>> Actually, I'm not 100% sure about this. I know this is Web Inspector style,
>> but maybe JSC uses another style for built ins. It's probably best to follow
>> JSC's style.
> 
> I see. So, I'll align to the builtin's code.

Ah, one question. Can I use the arrow function here? (I saw the lexical binding |this| patch is landed today).
Or it's not good for now because it has the flag?

[Saam Barati]

(In reply to comment #25)
> Comment on attachment 259101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259101&action=review
> 
> >>>>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:232
> >>>>> +    var fetchPromise = this.fetch(key).@then(function (payload) {
> >>>> 
> >>>> Nit: This and some other function expressions don't need a space between "function" and "(".
> >>> 
> >>> Ah, is it better to drop this space? I'll follow it.
> >> 
> >> Actually, I'm not 100% sure about this. I know this is Web Inspector style,
> >> but maybe JSC uses another style for built ins. It's probably best to follow
> >> JSC's style.
> > 
> > I see. So, I'll align to the builtin's code.
> 
> Ah, one question. Can I use the arrow function here? (I saw the lexical
> binding |this| patch is landed today).
> Or it's not good for now because it has the flag?

I think we should wait till arrow functions are fully completed before using
them inside JSC's builtins. That said, I like the style where arrow functions
are used only for very short functions. I think this function is better as a "function ..." function.

[Yusuke Suzuki]

Comment on attachment 259101 [details]
Patch

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

>>>>>>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:232
>>>>>>> +    var fetchPromise = this.fetch(key).@then(function (payload) {
>>>>>> 
>>>>>> Nit: This and some other function expressions don't need a space between "function" and "(".
>>>>> 
>>>>> Ah, is it better to drop this space? I'll follow it.
>>>> 
>>>> Actually, I'm not 100% sure about this. I know this is Web Inspector style,
>>>> but maybe JSC uses another style for built ins. It's probably best to follow
>>>> JSC's style.
>>> 
>>> I see. So, I'll align to the builtin's code.
>> 
>> Ah, one question. Can I use the arrow function here? (I saw the lexical binding |this| patch is landed today).
>> Or it's not good for now because it has the flag?
> 
> I think we should wait till arrow functions are fully completed before using
> them inside JSC's builtins. That said, I like the style where arrow functions
> are used only for very short functions. I think this function is better as a "function ..." function.

Make sense. So I continue to use "function" :)

[Yusuke Suzuki]

Comment on attachment 259101 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/ModuleLoaderObject.cpp:240
>>> +    if (!moduleRecord)
>> 
>> Is this the right thing to do here? Or should we throw an error?
>> Same question with checks in other hooks.
> 
> This is a little bit conservative guard. Since these methods only called from the builtin internal ModuleLoader object.
> So I can replace them with the assertion. I'll do so.

After considering about here, I think it's OK because it may take the optionalInstance in the future (this is not supported by the current implementation yet).
The existing spec seems that it does not integrate the optionalInstance well yet. So in the meantime, returning the empty array is the best solution I think.

[Yusuke Suzuki]

Created attachment 259367 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.h:151:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 30 files


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

[Yusuke Suzuki]

Comment on attachment 259367 [details]
Patch

The review comments are reflected. But the current patch is based on the InternalPromise patch[1].
So after the InternalPromise is landed, I'll check this patch and request the formal review.
But we can informally pre-review the patch.

[1]: Review comment is reflected. But the current patch is based on the r

[Yusuke Suzuki]

(In reply to comment #31)
> Comment on attachment 259367 [details]
> Patch
> 
> The review comments are reflected. But the current patch is based on the
> InternalPromise patch[1].
> So after the InternalPromise is landed, I'll check this patch and request
> the formal review.
> But we can informally pre-review the patch.
> 
> [1]: Review comment is reflected. But the current patch is based on the r

https://bugs.webkit.org/show_bug.cgi?id=148136

[Yusuke Suzuki]

Created attachment 259386 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.h:151:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 30 files


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

[Yusuke Suzuki]

The current submitted patch is based on the https://bugs.webkit.org/show_bug.cgi?id=148136.
All review comments are reflected I think :)
It can be built with https://bugs.webkit.org/show_bug.cgi?id=148136 and https://bugs.webkit.org/show_bug.cgi?id=148136#c7 tweaks. And I've ensured it works with the circular dependencies correctly.



==== tmp/main.js ====

import "tmp/test001.js"

==== tmp/test001.js ====

import 'tmp/arith.js'
import 'tmp/test001.js'

==== tmp/arith.js ====

import "tmp/test001.js"

export function add(a, b)
{
    return a + b;
}

$ Tools/Scripts/run-jsc --debug -m tmp/main.js
Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/yusukesuzuki/development/Modules/WebKitBuild/module-loader7-merge-promise/Debug /Users/yusukesuzuki/development/Modules/WebKitBuild/module-loader7-merge-promise/Debug/jsc -m tmp/main.js
Loader [translate] 
Loader [instantiate] 

Analyzing ModuleRecord
    Dependencies: 1 modules
      module('tmp/test001.js')
    Import: 0 entries
    Export: 0 entries
Loader [resolve] tmp/test001.js
Loader [fetch] tmp/test001.js
Loader [translate] tmp/test001.js
Loader [instantiate] tmp/test001.js

Analyzing ModuleRecord
    Dependencies: 2 modules
      module('tmp/arith.js')
      module('tmp/test001.js')
    Import: 0 entries
    Export: 0 entries
Loader [resolve] tmp/arith.js
Loader [resolve] tmp/test001.js
Loader [fetch] tmp/arith.js
Loader [translate] tmp/arith.js
Loader [instantiate] tmp/arith.js

Analyzing ModuleRecord
    Dependencies: 1 modules
      module('tmp/test001.js')
    Import: 0 entries
    Export: 1 entries
      [Local] export('add'), local('add')
Loader [resolve] tmp/test001.js

[Yusuke Suzuki]

Created attachment 259388 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.h:151:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 30 files


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

[Yusuke Suzuki]

Created attachment 259447 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.h:151:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 30 files


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

[Yusuke Suzuki]

Now InternalPromise is landed http://trac.webkit.org/changeset/188681
So I've updated the patch to submit it to the EWS.
The patch becomes ready :D

Detailed changes

+ InternalPromise is separated from the original patch. Now it just contains the change related to the ModuleLoader
+ Revised patch includes the fixes for the review comments.
+ Now we set the moduleKey as the origin (e.g. URL) of the source code, that's natural I think.

[Saam Barati]

(In reply to comment #35)
> The current submitted patch is based on the
> https://bugs.webkit.org/show_bug.cgi?id=148136.
> All review comments are reflected I think :)
> It can be built with https://bugs.webkit.org/show_bug.cgi?id=148136 and
> https://bugs.webkit.org/show_bug.cgi?id=148136#c7 tweaks. And I've ensured
> it works with the circular dependencies correctly.
> 
> 
> 
> ==== tmp/main.js ====
> 
> import "tmp/test001.js"
> 
> ==== tmp/test001.js ====
> 
> import 'tmp/arith.js'
> import 'tmp/test001.js'
> 
> ==== tmp/arith.js ====
> 
> import "tmp/test001.js"
> 
> export function add(a, b)
> {
>     return a + b;
> }
> 
> $ Tools/Scripts/run-jsc --debug -m tmp/main.js
> Running 1 time(s):
> DYLD_FRAMEWORK_PATH=/Users/yusukesuzuki/development/Modules/WebKitBuild/
> module-loader7-merge-promise/Debug
> /Users/yusukesuzuki/development/Modules/WebKitBuild/module-loader7-merge-
> promise/Debug/jsc -m tmp/main.js
> Loader [translate] 
> Loader [instantiate] 
> 
> Analyzing ModuleRecord
Can this dump name the module it's analyzing after "Analayzing ModuleRecord" like "Analyzing ModuleRecord <moduleName>"?
>     Dependencies: 1 modules
>       module('tmp/test001.js')
>     Import: 0 entries
>     Export: 0 entries
> Loader [resolve] tmp/test001.js
> Loader [fetch] tmp/test001.js
> Loader [translate] tmp/test001.js
> Loader [instantiate] tmp/test001.js
> 
> Analyzing ModuleRecord
>     Dependencies: 2 modules
>       module('tmp/arith.js')
>       module('tmp/test001.js')
>     Import: 0 entries
>     Export: 0 entries
> Loader [resolve] tmp/arith.js
> Loader [resolve] tmp/test001.js
> Loader [fetch] tmp/arith.js
> Loader [translate] tmp/arith.js
> Loader [instantiate] tmp/arith.js
> 
> Analyzing ModuleRecord
>     Dependencies: 1 modules
>       module('tmp/test001.js')
>     Import: 0 entries
>     Export: 1 entries
>       [Local] export('add'), local('add')
> Loader [resolve] tmp/test001.js

[Yusuke Suzuki]

(In reply to comment #41)
> Can this dump name the module it's analyzing after "Analayzing ModuleRecord"
> like "Analyzing ModuleRecord <moduleName>"?

Sure!

[Yusuke Suzuki]

Created attachment 259455 [details]
Patch

[Yusuke Suzuki]

Updated the patch with the moduleKey.
And the result of the dump.

Tools/Scripts/run-jsc --debug -m tmp/main.js  
Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/yusukesuzuki/development/Modules/WebKitBuild/module-loader8/Debug /Users/yusukesuzuki/development/Modules/WebKitBuild/module-loader8/Debug/jsc -m tmp/main.js
Loader [translate] EntryPointModule
Loader [instantiate] EntryPointModule

Analyzing ModuleRecord key(EntryPointModule)
    Dependencies: 1 modules
      module('tmp/test001.js')
    Import: 0 entries
    Export: 0 entries
Loader [resolve] tmp/test001.js
Loader [fetch] tmp/test001.js
Loader [translate] tmp/test001.js
Loader [instantiate] tmp/test001.js

Analyzing ModuleRecord key('tmp/test001.js')
    Dependencies: 2 modules
      module('tmp/arith.js')
      module('tmp/test001.js')
    Import: 0 entries
    Export: 0 entries
Loader [resolve] tmp/arith.js
Loader [resolve] tmp/test001.js
Loader [fetch] tmp/arith.js
Loader [translate] tmp/arith.js
Loader [instantiate] tmp/arith.js

Analyzing ModuleRecord key('tmp/arith.js')
    Dependencies: 1 modules
      module('tmp/test001.js')
    Import: 0 entries
    Export: 1 entries
      [Local] export('add'), local('add')
Loader [resolve] tmp/test001.js

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.h:155:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 30 files


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

[Yusuke Suzuki]

Created attachment 259490 [details]
Patch

Fix the rebase miss in project.pbxproj, the patch is ready.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSModuleRecord.h:155:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 30 files


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

[Saam Barati]

Comment on attachment 259490 [details]
Patch

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

r=me

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:32
> +// 1. Loader.resolve.

I think these would be more helpful at their call site. i.e, have this comment around or at where you call into the "resolve" hook.

> Source/JavaScriptCore/jsc.cpp:1386
> +            evaluateModule(globalObject->globalExec(), jscSource(script, fileName), evaluationException);

Can we also pass in the fileName to evaluateModule so we can make that the module name instead of ModuleEntryPoint?

> Source/JavaScriptCore/jsc.cpp:1390
> +#endif

maybe we should have an #else that prints an error message here.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:159
> +    typedef JSInternalPromise* (*LoaderResolvePtr)(JSGlobalObject*, ExecState*, JSValue, JSValue);

Can we prefix these with Module? So, ModuleLoaderResolvePtr in this case.

> Source/JavaScriptCore/runtime/ModuleLoaderObject.cpp:210
> +// ------------------------------ Hook Functions ---------------------------

Should this comment be lower down?

[Yusuke Suzuki]

Comment on attachment 259490 [details]
Patch

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

Thank you for your review, Saam :D

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:32
>> +// 1. Loader.resolve.
> 
> I think these would be more helpful at their call site. i.e, have this comment around or at where you call into the "resolve" hook.

Make sense. I'll move these comments to the call site.

>> Source/JavaScriptCore/jsc.cpp:1386
>> +            evaluateModule(globalObject->globalExec(), jscSource(script, fileName), evaluationException);
> 
> Can we also pass in the fileName to evaluateModule so we can make that the module name instead of ModuleEntryPoint?

Yup! I'm now working on it with the asynchronous error report, so I'll submit it in the subsequent patch.

>> Source/JavaScriptCore/jsc.cpp:1390
>> +#endif
> 
> maybe we should have an #else that prints an error message here.

We can pass the `-m` that makes `bool module = true` only when compiling JSC with ENABLE_ES6_MODULES.
So when the `module` is true, `ENABLE(ES6_MODULES)` is always true.

Because of the above reason, I'll insert the following here.
#else
RELEASE_ASSERT_NOT_REACHED()
#endif

>> Source/JavaScriptCore/runtime/JSGlobalObject.h:159
>> +    typedef JSInternalPromise* (*LoaderResolvePtr)(JSGlobalObject*, ExecState*, JSValue, JSValue);
> 
> Can we prefix these with Module? So, ModuleLoaderResolvePtr in this case.

Sure! I'll fix them.

>> Source/JavaScriptCore/runtime/ModuleLoaderObject.cpp:210
>> +// ------------------------------ Hook Functions ---------------------------
> 
> Should this comment be lower down?

Nice catch. Oops. I'll fix this.

[Yusuke Suzuki]

Committed r188752: <http://trac.webkit.org/changeset/188752>