Bug 191121 – JSC should have a module loader API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

JSC should have a module loader API

[Keith Miller]

Created attachment 353502 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1823:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 12 files


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

[Keith Miller]

rdar://problem/31376275

[Geoffrey Garen]

Comment on attachment 353502 [details]
Patch

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

> Source/JavaScriptCore/API/JSAbstractModule.h:33
> +@interface JSAbstractModule : NSObject

I'm not clear on what "Abstract" means here. There is no JSConcreteModule. Maybe this should just be JSModule.

> Source/JavaScriptCore/API/JSAbstractModule.h:36
> ++ (instancetype)moduleWithSource:(NSString *)source andURL:(NSURL *)url inVirtualMachine:(JSVirtualMachine *)vm;

I think we usually leave out "and" and "in" in method names.

> Source/JavaScriptCore/API/JSAbstractModule.h:38
> +// Add more constructors for various ways to build modules e.g. builtin modules.

Not really appropriate for an API header.

> Source/JavaScriptCore/API/JSContext.h:83
> +- (JSValue *)evaluteModule:(JSAbstractModule *);

It's kind of odd that a module is just a script and a URL, but somehow that's different from evaulateScript:withSourceURL:.

Maybe evaluateModule should have the same API as evaluateScript, but it returns a module object. Then the JSModule class wouldn't need to allow you to construct a module directly.

> Source/JavaScriptCore/API/JSVirtualMachine.h:57
> +- (instancetype)initWithModuleLoader:(JSModuleLoaderResolutionCallback)loaderCallback;

It's usually better for a client like this to be an object. That way, you can add client methods in the future if module loader resolution ever becomes more complicated.

Should we offer a getter/setter for the VM's module loader resolution callback?

[Keith Miller]

Created attachment 359040 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1847:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/JSScript.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 25 files


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

[Keith Miller]

Created attachment 359041 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1847:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/JSScript.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 25 files


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

[Keith Miller]

Created attachment 359044 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1847:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/JSScript.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 26 files


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

[Keith Miller]

Created attachment 359048 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1847:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/JSScript.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 27 files


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

[Keith Miller]

Created attachment 359050 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1847:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/JSScript.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 27 files


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

[Keith Miller]

Created attachment 359053 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1847:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/JSScript.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 27 files


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

[Keith Miller]

Created attachment 359054 [details]
Patch

[Dean Jackson]

Comment on attachment 359050 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        This patch adds a new delegate to JSContxt that is called to fetch

typo JSContext

> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:117
> +    if (!specifier.startsWith('/') && !specifier.startsWith("./") && !specifier.startsWith("../"))
> +        return reject(createError(exec, "Module specifier does not start with \"/\", \"./\", or \"../\"."_s));

why doesn't ES Modules allow you to import { Foo } from "Foo.js" without the "./Foo.js"? I think the modules in Node allow this.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1847:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/JSScript.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 27 files


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

[Michael Saboff]

Comment on attachment 359054 [details]
Patch

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

r=me after EWS is green.

> Source/JavaScriptCore/API/JSContextPrivate.h:37
> +/*! @abstract Provides source code for any JS module that is actively imported.

Shouldn't there be an @method and @abstract on its own line.

> Source/JavaScriptCore/API/JSContextPrivate.h:74
> +/*! @abstract The delegate the context will use when trying to load a module. Note, this delegate will be ignored for contexts returned by UIWebView. */

Shouldn't @property be above @abstract and both on their own lines.

[Keith Miller]

Comment on attachment 359050 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        This patch adds a new delegate to JSContxt that is called to fetch
> 
> typo JSContext

Fixed.

>> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:117
>> +        return reject(createError(exec, "Module specifier does not start with \"/\", \"./\", or \"../\"."_s));
> 
> why doesn't ES Modules allow you to import { Foo } from "Foo.js" without the "./Foo.js"? I think the modules in Node allow this.

I don't think it's an ES Modules thing. The ES spec lets you do whatever you want with your specifier/importer pair. The web makes this restriction for future proofing. 

As far as node goes. it seems like import "foo.js" will never resolve whereas import "./foo.js" seems to work. Maybe I'm doing it wrong though...

[Keith Miller]

Created attachment 359058 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1847:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/JSScript.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 28 files


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

[Keith Miller]

Created attachment 359060 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1847:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/JSScript.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 29 files


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

[Keith Miller]

Comment on attachment 359054 [details]
Patch

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

>> Source/JavaScriptCore/API/JSContextPrivate.h:37
>> +/*! @abstract Provides source code for any JS module that is actively imported.
> 
> Shouldn't there be an @method and @abstract on its own line.

I don't think so. At least that's not what we do in WKWebView's delegate protocols.

>> Source/JavaScriptCore/API/JSContextPrivate.h:74
>> +/*! @abstract The delegate the context will use when trying to load a module. Note, this delegate will be ignored for contexts returned by UIWebView. */
> 
> Shouldn't @property be above @abstract and both on their own lines.

Ditto.

[Keith Miller]

Committed r239933: <https://trac.webkit.org/changeset/239933>

[Geoffrey Garen]

Comment on attachment 359060 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        any resolved module. The resolution of a module identifier is computed
> +        as if it were a URL on the web with the caveat that it must be a file URL.

Why limit to file URLs? Seems like we should be able to resolve HTTP URLs too, and clients might want that.

> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:125
> +    if (specifier.startsWith('/')) {
> +        absoluteURL = URL(URL({ }, "file://"), specifier);
> +        if (absoluteURL.isValid())
> +            return import(absoluteURL);
> +    }

Seems like you should just use the sourceOrigin here instead of forcing file://. Client apps may host modules on the web.

> Source/JavaScriptCore/API/JSContextPrivate.h:50
> +- (void)context:(JSContext *)context fetchModuleForIdentifier:(JSValue *)identifier withResolveHandler:(JSValue *)resolve andRejectHandler:(JSValue *)reject;

Delegate APIs usually supply blocks for callbacks, rather than JSValues. That's a more familiar syntax for Swift and Objective-C programmers. 

Relatedly, why is the module identifier a JSValue instead of a URL?

Has anybody specifically requested the ability to inspect a resolve handler, a reject handler, or a module identifier as a JavaScript value? What benefit would that provide? In addition to fitting in with the platform better overall, platform data types come with their own documentation, and don't require an explanation. NSURL * is self-documenting. JSValue * with three lines of text is a headache. I would find this API much easier to understand and use if it just passed me an NSURL * or NSString * and two blocks.

[Keith Miller]

Comment on attachment 359060 [details]
Patch

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

>> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:125
>> +    }
> 
> Seems like you should just use the sourceOrigin here instead of forcing file://. Client apps may host modules on the web.

SourceOrigin may be null. In that case it's not clear what URI to use. In theory we could make this not

>> Source/JavaScriptCore/API/JSContextPrivate.h:50
>> +- (void)context:(JSContext *)context fetchModuleForIdentifier:(JSValue *)identifier withResolveHandler:(JSValue *)resolve andRejectHandler:(JSValue *)reject;
> 
> Delegate APIs usually supply blocks for callbacks, rather than JSValues. That's a more familiar syntax for Swift and Objective-C programmers. 
> 
> Relatedly, why is the module identifier a JSValue instead of a URL?
> 
> Has anybody specifically requested the ability to inspect a resolve handler, a reject handler, or a module identifier as a JavaScript value? What benefit would that provide? In addition to fitting in with the platform better overall, platform data types come with their own documentation, and don't require an explanation. NSURL * is self-documenting. JSValue * with three lines of text is a headache. I would find this API much easier to understand and use if it just passed me an NSURL * or NSString * and two blocks.

I made it a JSValue callback to make it explicit that it will re-enter the VM per our discussion.

The identifier is a JSValue rather an a URL as it's significantly more future proof. If, for example builtin modules become a thing, we need some way to pass that onto the fetcher -- for polyfilling. There is also nothing in the JS language that restricts identifiers from extending beyond strings. In which case, we'd be in the awkward situation of having to add a second delegate that handles that case. Given that module loading is still heavy active in every committee (TC-39, W3C, Node) that's a real risk.

[Geoffrey Garen]

> > Seems like you should just use the sourceOrigin here instead of forcing file://. Client apps may host modules on the web.
> 
> SourceOrigin may be null. In that case it's not clear what URI to use. In
> theory we could make this not

We can check for null.

More broadly, if someone imports an absolute or relative path relative to an invalid host, scheme, or port, I recommend that we try our best to resolve relative to the host, scheme, and port specified by our client, and from their, it's their job to handle the resulting URL, or fail the module load.

At the limit, if we know for certain that the resulting URL is invalid, we can keep your current behavior to fail eagerly -- or, if you like, we can send the invalid URL to our client and let them sort it out.

> >> Source/JavaScriptCore/API/JSContextPrivate.h:50
> >> +- (void)context:(JSContext *)context fetchModuleForIdentifier:(JSValue *)identifier withResolveHandler:(JSValue *)resolve andRejectHandler:(JSValue *)reject;
> > 
> > Delegate APIs usually supply blocks for callbacks, rather than JSValues. That's a more familiar syntax for Swift and Objective-C programmers. 
> > 
> > Relatedly, why is the module identifier a JSValue instead of a URL?
> > 
> > Has anybody specifically requested the ability to inspect a resolve handler, a reject handler, or a module identifier as a JavaScript value? What benefit would that provide? In addition to fitting in with the platform better overall, platform data types come with their own documentation, and don't require an explanation. NSURL * is self-documenting. JSValue * with three lines of text is a headache. I would find this API much easier to understand and use if it just passed me an NSURL * or NSString * and two blocks.
> 
> I made it a JSValue callback to make it explicit that it will re-enter the
> VM per our discussion.

It's usually understood, in delegate APIs, that once the delegate returns, or invokes a provided callback, the framework will then continue executing code.

Also, I don't think "JSValue" means "JavaScriptCore has an internal locking strategy". I think that's just JavaScriptCore's behavior for all its APIs, JSValue or not.

So, I think this should be a block.

You could convince me otherwise by citing a prominent example of a JavaScript app that is somehow hamstrung by a block.

> The identifier is a JSValue rather an a URL as it's significantly more
> future proof. If, for example builtin modules become a thing, we need some
> way to pass that onto the fetcher -- for polyfilling. There is also nothing
> in the JS language that restricts identifiers from extending beyond strings.
> In which case, we'd be in the awkward situation of having to add a second
> delegate that handles that case. Given that module loading is still heavy
> active in every committee (TC-39, W3C, Node) that's a real risk.

Though the ECMA specification has some weirdness in it, that's not how people experience the programming language. We're trying to create an environment that papers over the weirdness of the specification and provides the Javascript programming environment in a way that's easy to use right by default, and hard to use wrong.

So, I think we should use URL here.

You could convince me otherwise by citing a prominent example of a JavaScript app that is somehow hamstrung by a URL.