Bug 191121

Summary: JSC should have a module loader API
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, ggaren, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Keith Miller 2018-10-31 10:43:37 PDT
JSC should have a module loader API
Comment 1 Keith Miller 2018-10-31 10:44:38 PDT
Created attachment 353502 [details]
Patch
Comment 2 EWS Watchlist 2018-10-31 12:05:03 PDT
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.
Comment 3 Keith Miller 2018-11-07 11:23:37 PST
rdar://problem/31376275
Comment 4 Geoffrey Garen 2018-11-27 21:20:02 PST
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?
Comment 5 Keith Miller 2019-01-14 09:12:52 PST
Created attachment 359040 [details]
Patch
Comment 6 EWS Watchlist 2019-01-14 09:16:22 PST
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.
Comment 7 Keith Miller 2019-01-14 09:19:31 PST
Created attachment 359041 [details]
Patch
Comment 8 EWS Watchlist 2019-01-14 09:21:44 PST
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.
Comment 9 Keith Miller 2019-01-14 09:41:55 PST
Created attachment 359044 [details]
Patch
Comment 10 EWS Watchlist 2019-01-14 09:45:26 PST
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.
Comment 11 Keith Miller 2019-01-14 09:59:29 PST
Created attachment 359048 [details]
Patch
Comment 12 EWS Watchlist 2019-01-14 10:03:40 PST
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.
Comment 13 Keith Miller 2019-01-14 10:28:41 PST
Created attachment 359050 [details]
Patch
Comment 14 EWS Watchlist 2019-01-14 10:30:22 PST
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.
Comment 15 Keith Miller 2019-01-14 10:51:48 PST
Created attachment 359053 [details]
Patch
Comment 16 EWS Watchlist 2019-01-14 10:54:35 PST
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.
Comment 17 Keith Miller 2019-01-14 11:04:11 PST
Created attachment 359054 [details]
Patch
Comment 18 Dean Jackson 2019-01-14 11:06:30 PST
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.
Comment 19 EWS Watchlist 2019-01-14 11:07:13 PST
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.
Comment 20 Michael Saboff 2019-01-14 11:13:42 PST
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.
Comment 21 Keith Miller 2019-01-14 11:23:17 PST
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...
Comment 22 Keith Miller 2019-01-14 11:34:23 PST
Created attachment 359058 [details]
Patch
Comment 23 EWS Watchlist 2019-01-14 11:37:35 PST
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.
Comment 24 Keith Miller 2019-01-14 11:42:46 PST
Created attachment 359060 [details]
Patch
Comment 25 EWS Watchlist 2019-01-14 11:46:10 PST
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.
Comment 26 Keith Miller 2019-01-14 12:23:07 PST
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.
Comment 27 Keith Miller 2019-01-14 12:27:29 PST
Committed r239933: <https://trac.webkit.org/changeset/239933>
Comment 28 Geoffrey Garen 2019-01-15 20:58:26 PST
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.
Comment 29 Keith Miller 2019-01-24 11:37:36 PST
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.
Comment 30 Geoffrey Garen 2019-02-05 12:45:21 PST
> > 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.