Bug 147876 - [ES6] prototyping module loader in JSC shell
Summary: [ES6] prototyping module loader in JSC shell
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 147340 148053
  Show dependency treegraph
 
Reported: 2015-08-11 02:23 PDT by Yusuke Suzuki
Modified: 2015-08-20 22:00 PDT (History)
8 users (show)

See Also:


Attachments
Patch (111.53 KB, patch)
2015-08-14 17:49 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (130.28 KB, patch)
2015-08-15 01:05 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (142.56 KB, patch)
2015-08-15 02:28 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (142.59 KB, patch)
2015-08-15 12:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (142.62 KB, patch)
2015-08-15 13:08 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (124.17 KB, patch)
2015-08-19 00:53 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (123.97 KB, patch)
2015-08-19 11:54 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (124.18 KB, patch)
2015-08-19 12:02 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (124.24 KB, patch)
2015-08-19 21:52 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (124.80 KB, patch)
2015-08-20 00:32 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (123.76 KB, patch)
2015-08-20 14:04 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-08-11 02:23:00 PDT
Let's implement the synchronous module loader in JSC shell side to test the dependency analysis.
Comment 1 Yusuke Suzuki 2015-08-12 20:03:38 PDT
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/
Comment 2 Yusuke Suzuki 2015-08-13 17:01:58 PDT
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.
Comment 3 Yusuke Suzuki 2015-08-14 17:49:23 PDT
Created attachment 259060 [details]
Patch

WIP
Comment 4 Yusuke Suzuki 2015-08-14 20:18:03 PDT
Opened the issues related to this.

https://github.com/whatwg/loader/pull/66
https://github.com/whatwg/loader/pull/67
Comment 5 Yusuke Suzuki 2015-08-15 00:09:25 PDT
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".
Comment 6 Yusuke Suzuki 2015-08-15 01:05:47 PDT
Created attachment 259085 [details]
Patch

WIP
Comment 7 Yusuke Suzuki 2015-08-15 01:26:27 PDT
https://github.com/whatwg/loader/issues/68 Proposed fix.
Comment 8 WebKit Commit Bot 2015-08-15 01:59:59 PDT
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.
Comment 9 Yusuke Suzuki 2015-08-15 02:28:55 PDT
Created attachment 259087 [details]
Patch
Comment 10 WebKit Commit Bot 2015-08-15 02:31:38 PDT
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.
Comment 11 Yusuke Suzuki 2015-08-15 12:48:36 PDT
Created attachment 259100 [details]
Patch
Comment 12 WebKit Commit Bot 2015-08-15 12:51:22 PDT
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.
Comment 13 Yusuke Suzuki 2015-08-15 13:08:07 PDT
Created attachment 259101 [details]
Patch
Comment 14 WebKit Commit Bot 2015-08-15 13:10:52 PDT
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.
Comment 15 Yusuke Suzuki 2015-08-15 16:00:35 PDT
ok, pstch is ready.
Comment 16 Saam Barati 2015-08-15 18:39:57 PDT
(In reply to comment #15)
> ok, pstch is ready.
Okay. I can review tomorrow night if you want.
Comment 17 Yusuke Suzuki 2015-08-15 21:29:17 PDT
(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
Comment 18 Yusuke Suzuki 2015-08-15 23:30:59 PDT
(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.
Comment 19 Saam Barati 2015-08-17 19:58:34 PDT
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.
Comment 20 Yusuke Suzuki 2015-08-17 20:54:40 PDT
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.
Comment 21 Saam Barati 2015-08-17 22:35:26 PDT
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.
Comment 22 Yusuke Suzuki 2015-08-17 22:56:28 PDT
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.
Comment 23 Saam Barati 2015-08-17 22:58:43 PDT
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.
Comment 24 Yusuke Suzuki 2015-08-17 23:00:39 PDT
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.
Comment 25 Yusuke Suzuki 2015-08-17 23:02:10 PDT
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?
Comment 26 Saam Barati 2015-08-18 23:56:12 PDT
(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.
Comment 27 Yusuke Suzuki 2015-08-19 00:26:50 PDT
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" :)
Comment 28 Yusuke Suzuki 2015-08-19 00:49:11 PDT
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.
Comment 29 Yusuke Suzuki 2015-08-19 00:53:45 PDT
Created attachment 259367 [details]
Patch
Comment 30 WebKit Commit Bot 2015-08-19 00:55:45 PDT
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.
Comment 31 Yusuke Suzuki 2015-08-19 00:55:52 PDT
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
Comment 32 Yusuke Suzuki 2015-08-19 00:56:38 PDT
(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
Comment 33 Yusuke Suzuki 2015-08-19 11:54:29 PDT
Created attachment 259386 [details]
Patch
Comment 34 WebKit Commit Bot 2015-08-19 11:57:05 PDT
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.
Comment 35 Yusuke Suzuki 2015-08-19 11:58:03 PDT
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
Comment 36 Yusuke Suzuki 2015-08-19 12:02:52 PDT
Created attachment 259388 [details]
Patch
Comment 37 WebKit Commit Bot 2015-08-19 12:04:57 PDT
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.
Comment 38 Yusuke Suzuki 2015-08-19 21:52:43 PDT
Created attachment 259447 [details]
Patch
Comment 39 WebKit Commit Bot 2015-08-19 21:54:59 PDT
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.
Comment 40 Yusuke Suzuki 2015-08-19 21:57:58 PDT
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.
Comment 41 Saam Barati 2015-08-19 23:52:18 PDT
(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
Comment 42 Yusuke Suzuki 2015-08-20 00:18:39 PDT
(In reply to comment #41)
> Can this dump name the module it's analyzing after "Analayzing ModuleRecord"
> like "Analyzing ModuleRecord <moduleName>"?

Sure!
Comment 43 Yusuke Suzuki 2015-08-20 00:32:40 PDT
Created attachment 259455 [details]
Patch
Comment 44 Yusuke Suzuki 2015-08-20 00:33:35 PDT
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
Comment 45 WebKit Commit Bot 2015-08-20 00:34:42 PDT
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.
Comment 46 Yusuke Suzuki 2015-08-20 14:04:55 PDT
Created attachment 259490 [details]
Patch

Fix the rebase miss in project.pbxproj, the patch is ready.
Comment 47 WebKit Commit Bot 2015-08-20 14:06:57 PDT
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.
Comment 48 Saam Barati 2015-08-20 18:15:21 PDT
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?
Comment 49 Yusuke Suzuki 2015-08-20 20:33:12 PDT
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.
Comment 50 Yusuke Suzuki 2015-08-20 22:00:08 PDT
Committed r188752: <http://trac.webkit.org/changeset/188752>