Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discussion] Simplification of Loader #147

Open
caridy opened this issue Jun 30, 2016 · 45 comments
Open

[Discussion] Simplification of Loader #147

caridy opened this issue Jun 30, 2016 · 45 comments

Comments

@caridy
Copy link
Contributor

caridy commented Jun 30, 2016

Disclaimer: the following is a compilation of many thoughts and discussions about the future of the loader spec, it is not an actual plan, just food for thoughts.

Rationale on why we might want to simplify the Loader API

  • Importing relative and global modules on-demand is a primary use-case to level-up with node/CJS capabilities (import() imperative form is needed).
  • A module may want to create a new loader instance via new Reflect.Loader(), useful for frameworks and sandboxes.
  • A module should never attempt to use System.loader because it makes the module less portable.
  • The default global loader instance (e.g.: System.loader) is rarely needed.
  • <script type="module"> or node's --module (or it equivalent) are far better options to kick in your app than the global loader.
  • fetch and translate hooks can be implemented in user-land via service workers or Realm's evaluation hook.
  • instantiate hook can be removed as well if the registry is exposed via the current loader reference.

Examples

Imperative vs Declarative Import

Imperative form of import, equivalent to declarative import statement:

import {something} from "./foo.js";
something(1);

import("./foo.js").then(foo => ({something}) {
    something(2);
});

Even easier when using await:

const {something} = await import("foo");
something(2);

note: the catch is that something may or may not be a live binding depending on how you import it.

Configuring Loader

Extending the current loader via hooks:

import {resolveHook} from "custom-resolver";
import.loader[Reflect.Loader.resolve] = resolveHook;
import("foo").then(foo => ({something}) {
    something(2);
});

note: the catch here is that module "custom-resolver" and its dependencies are imported before attempting to configure the loader, while "foo" and its dependencies are imported after the fact, which means they will be subject to the new resolution rules defined by the new hook.

Similarly, you can apply AOP on current resolver hooks:

import {resolveFactory} from "custom-resolver";
const oldResolver = import.loader[Reflect.Loader.resolve];
import.loader[Reflect.Loader.resolve] = resolveFactory(oldResolver);
import("foo").then(foo => ({something}) {
    something(2);
});

Controlling the Registry

If we open up the resolve hook, then we will have to open on the registry as well:

const loader = import.loader;
loader[Reflect.Loader.resolve] = (name, referrer) => {
    if (name === "foo" && !loader.registry.has(name)) {
        const fooNS = new Reflect.Module(...);
        loader.registry.set(name, fooNS);
    }
    return name;
};
import("foo").then(foo => {
    foo; // ... it is just a reference to the exotic object `fooNS` created in the resolver.
});

note: this also show that the @@instantiate hook may not be needed after all.

Default Loader

Not need to have a System.loader initially, instead, you can use <script type="module"> to gain access to the global loader, e.g:

<script type="module">
const loader = import.loader;
</script>

Custom Loader

A module may want to create a loader instance, pre-populate its registry, configure the hook, and start importing other modules using the newly created loader instance, e.g.:

<script type="module">
const l1 = new Reflect.loader();
// configure the loader in any way you want
l1.import('foo').then(...);
</script>

note: "foo" module and its dependencies will have access to l1 via import.loader.

Open questions

Should the current loader be exposed initially?

const loader = import.loader;
export {loader};

note: it seems that this is needed to simplify the mental model of the example above.

What about other intermedia states for relative modules?

We may want to expose a resolve and load mechanism for relative modules, e.g.:

// relative vs top level?
import.resolve('./foo'); // the referrer (2nd arg) is implicit
import.loader.resolve('foo'); // top level resolve operation
// and load?
import.load('./foo');      // relative
import.loader.load('foo'); // top level

note: the resolve() and load() methods are important to apply performance optimizations.

Alternative, if the key of the module in the loader registry is exposed somehow, then we might not need relative resolve and relative load after all. e.g.:

import.loader.resolve('./foo', import.key); // relative
import.loader.resolve('foo');               // top level resolve operation
import.load('./foo', import.key);           // relative
import.loader.load('foo');                  // top level load operation

note: this second option is probably better because it retains the mental model of dealing with the loader power object.

@caridy
Copy link
Contributor Author

caridy commented Jun 30, 2016

/cc @wycats @dherman @ajklein @bterlson

@caridy
Copy link
Contributor Author

caridy commented Jun 30, 2016

Removing the fetch hook will solve #132

@caridy
Copy link
Contributor Author

caridy commented Jun 30, 2016

related to #121, #69 and #130

@caridy
Copy link
Contributor Author

caridy commented Jun 30, 2016

this will also answer the question from #72, since there will be no global loader to be mutated.

@caridy
Copy link
Contributor Author

caridy commented Jun 30, 2016

it also touch on the imperative form of import defined in #36

@caridy
Copy link
Contributor Author

caridy commented Jun 30, 2016

this might entirely solve #89

@domenic
Copy link
Member

domenic commented Jun 30, 2016

This seems like a great direction to me.

Further simplification may be achievable by not allowing mutation of import.loader. instead you load scripts with custom loaders using thatLoader.import (which also applies to their dependencies).

That way you can't affect the way other code loads, unless you yourself load it.

@matthewp
Copy link

matthewp commented Jul 1, 2016

In regards to the fetch hook and using service workers, would there be a way to know, in the service worker fetch event, that the FetchEvent is coming from a loader request (vs. an XHR request, a <link> request, etc.)? That would be important to know in order to do the types of things that you are likely to do in a loader fetch or translate hook.

@guybedford
Copy link

Nice to see the simplification effort here! Is 'import.loader.load' here
just a normal import function?
On Fri, 01 Jul 2016 at 02:11, Matthew Phillips notifications@github.com
wrote:

In regards to the fetch hook and using service workers, would there be a
way to know, in the service worker fetch event, that the FetchEvent is
coming from a loader request (vs. an XHR request, a request, etc.)? That
would be important to know in order to do the types of things that you are
likely to do in a loader fetch or translate hook.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#147 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAkiyvg05xg2E8pB-Uj0bXQgWZZiLIVPks5qRFtDgaJpZM4JCrlP
.

@caridy
Copy link
Contributor Author

caridy commented Jul 1, 2016

@guybedford no, import.loader.load is just loader.load(), which is available today to fetch a module and its dependencies, instantiate all of them, and get them ready to be evaluated.

@guybedford
Copy link

Thanks @caridy that makes sense. Will loader.define still be available for
population of modular source text? Would be necessary to allow translation
workflows to esm I think?
On Fri, 01 Jul 2016 at 15:49, Caridy Patiño notifications@github.com
wrote:

@guybedford https://github.com/guybedford no, import.loader.load is
just loader.load(), which is available today to fetch a module and its
dependencies, instantiate all of them, and get them ready to be evaluated.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#147 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAkiysV34houNttd0IycMJ9MFrQuT81Gks5qRRrogaJpZM4JCrlP
.

@caridy
Copy link
Contributor Author

caridy commented Jul 1, 2016

@guybedford no, you can push module records into the registry (check the example "Controlling the Registry"), but as today, there is no way to process a source text module record from the loader. This might or might not be a problem, we have some ideas. Alternative, you can always rely on the service worker to produce the string to be evaluated for a particular key/url, in which case the loader will evaluate it.

@guybedford
Copy link

That does remove some use cases for in-browser transpilation eg macros for
modules. While it can be done in the service worker, a modular 'eval'
through loader.define to a given module name would be ideal to properly
support this.
On Fri, 01 Jul 2016 at 16:46, Caridy Patiño notifications@github.com
wrote:

@guybedford https://github.com/guybedford no, you can push module
records into the registry (check the example "Controlling the Registry"),
but as today, there is no way to process a source text module record from
the loader. This might or might not be a problem, we have some ideas.
Alternative, you can always rely on the service worker to produce the
string to be evaluated for a particular key/url, in which case the loader
will evaluate it.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#147 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAkiyuOwnvImcJXI1ncmtI2nCIG5CiRHks5qRShAgaJpZM4JCrlP
.

@caridy
Copy link
Contributor Author

caridy commented Jul 1, 2016

@guybedford there is not such things as a module name in loader, but a registry key. My position is that the loader doesn't need to give you such mechanism, just like it doesn't let you create a new reflective module record, you do that via new Reflect.Module(), and then you push the exotic namespace object into the registry. Similarly, we could have a way to create a Source Text Module Record from a source, returning a namespace object to userland. Whether that's via a constructor like Reflect.Module, or via a Realm or any other mechanism, I don't know.

@guybedford
Copy link

Right, I mean key instead of module name in the above. Ideally modular eval
is a function of the key and source text, returning a promise that acts
just like loader.load. Loading and execution can then happen according to
the normal spec support, which isn't possible if we want to get back an
executed record immediately. I don't know what API is best either - just
commenting on the fact that modular eval is now not possible.
On Fri, 01 Jul 2016 at 17:09, Caridy Patiño notifications@github.com
wrote:

@guybedford https://github.com/guybedford there is not such things as a
module name in loader, but a registry key. My position is that the loader
doesn't need to give you such mechanism, just like it doesn't let you
create a new reflective module record, you do that via new
Reflect.Module(), and then you push the exotic namespace object into the
registry. Similarly, we could have a way to create a Source Text Module
Record from a source, returning a namespace object to userland. Whether
that's via a constructor like Reflect.Module, or via a Realm or any other
mechanism, I don't know.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#147 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAkiyvaUE5Wo4EXGxjNEaEcEVIjH_Rk3ks5qRS2agaJpZM4JCrlP
.

@matthewp
Copy link

matthewp commented Jul 1, 2016

@caridy I thought ModuleStatus.prototype.resolve was the new loader.define equivalent, is that wrong or are you thinking about removing it?

@caridy
Copy link
Contributor Author

caridy commented Jul 1, 2016

@matthewp ModuleStatus.prototype.resolve just return the promise of the resolve hook, that's all. What @guybedford is claiming is that by removing the other hooks, you loose the ability to feed the loader with a source text in a form of a string value that the loader will use to call ParseModule abstract operation, which is an accurate claim.

@guybedford
Copy link

guybedford commented Jul 5, 2016

I have one other concern about this proposal - resolve will now execute as a side-effect for legacy module format cases. But the issue here is that since the loader is available to modules I can write portable code that uses this resolve function, and won't know whether my resolve will happen to result in an execution side-effect or not.

For example:

import {x} from 'y';

import.resolve('q').then(function(resolved) {
  // I really didn't mean to intend execution of q with the above code
  // but if q is a dynamic module, then it would have executed by now!
});

The other issue is that it is no longer possible for dynamic modules to execute within their ordering relative to the ES module tree by providing a function output from instantiate. This can be useful for cases like:

import 'es-module-1';
import 'cjs-module';
import 'es-module-2';

Where we want import ordering to apply so that es-module-1 executes before cjs-module (given no other tree dependencies present).

If we were to still retain the instantiate function, then we'd still have the ability to provide these kinds of cases.

Update: because Module objects have a separate evaluate stage, the above execution ordering can be retained. So it is only the resolve argument above that still stands here.

Let me know if anything in the above is unclear though.

@Alphapage
Copy link

I don't understand why metadata is only available after resolve: I think it should be available asap during each hook.
Instantiate could help to do some kind of preload. I think for now, it is impossible to resolve, fetch, translate and stop code execution. Instantiate could help to put a module in 'standby' and execute later when needed.

@caridy
Copy link
Contributor Author

caridy commented Jul 7, 2016

@guybedford

resolve will now execute as a side-effect for legacy module format cases

This is a very good point, yes, resolver hook will have to resolve, inspect and set up something, even when we don't plan to use it, fair enough.

it is no longer possible for dynamic modules to execute within their ordering relative to the ES module tree

I'm not sure this is 100% accurate. For legacy modules, you can still create a reflective module record, passing an evaluator function to apply lazy evaluation, which is going to be triggered just like source text module record evaluation. Also, there is not such thing as a deterministic evaluation order of your dependencies (because they might or might not be already evaluated by another turn), the only thing that is deterministic here is that the dependencies of a module will be evaluated before the module is evaluated, and when circular dependencies are present, this is also deterministic based on the root module to be imported.

@shicks
Copy link

shicks commented Jul 8, 2016

Is there a compelling reason to invent the new syntax of having import be indexable and callable? At the very least, this makes it much more difficult to polyfill, and seems gratuitous when it could just as easily be System.import which is valid ES and doesn't require changes to the language.

@matthewp
Copy link

matthewp commented Jul 9, 2016

System is a global where import.loader presumably refers to the loader that loaded the module.

@guybedford
Copy link

@caridy

I'm not sure this is 100% accurate. For legacy modules, you can still create a reflective module record, passing an evaluator function to apply lazy evaluation, which is going to be triggered just like source text module record evaluation. Also, there is not such thing as a deterministic evaluation order of your dependencies (because they might or might not be already evaluated by another turn), the only thing that is deterministic here is that the dependencies of a module will be evaluated before the module is evaluated, and when circular dependencies are present, this is also deterministic based on the root module to be imported.

I'm still not exactly clear on this API. Are you saying I could write something like the following:

loader[Reflect.loader.resolve] = function(key, parent) {
  var exports;
  loader.set('some/cjs/module.js', new Reflect.Module({
    default: { value:  null }
  }, function executor(mutator, module) {
    exports = mutator;
  }, function evaluate() {
    console.log('CJS evaluated');
    exports.default = 'evaluated';
    // CJS loaded from CJS would trigger require execution down the stack here I assume?
  });

  return resolve(key, parent);
};

Where if the module at resolve(key, parent) were to contain:

import './first-es-module.js';
import cjs from 'some/cjs/module.js';
console.log('CJS module from ES module is ' + cjs);

We would get first-es-module.js executing before some/cjs/module.js outputting CJS evaluated before finally getting the output CJS module from ES module is evaluated? If so I guess it does capture the instantiate utility through the module record process?

@caridy
Copy link
Contributor Author

caridy commented Jul 11, 2016

@guybedford it is not exactly like that (what you set into the registry are instance of ModuleStatus), but you get the idea right. and yes, it does preserve the order of evaluation. this is ideal to support circular dependencies, and all other features of the ES Modules.

@guybedford
Copy link

Thanks @caridy for the clarification.

The remaining issue here is then that CommonJS that loads ES modules would need to execute those ES modules within the resolve hook itself (effectively what was the zebra striping).

So even with this execution separation, you'd still need execution within the resolve hook for those cross dependencies, which may still be enough to justify a separate instantiation hook which just separates the phases.

@caridy
Copy link
Contributor Author

caridy commented Jul 13, 2016

@guybedford correct, it will only work if you already have the CJS module ready to be used before the resolve hook gets invoked (loaded as browserify/webpack bundle of something), or load them sync from disk in node.

@matthewp
Copy link

matthewp commented Jul 14, 2016

Maybe resolve can be moved out into service workers as well. It doesn't make sense that JavaScript modules will have a resolve but html imports will not. It should be generic. If the "fetch" event added some more information; the document.baseURI, and the identifier (./foo.js) for example then resolve wouldn't be needed window-side and we'd gain the benefit of resolving non-JavaScript stuff.

@domenic
Copy link
Member

domenic commented Jul 14, 2016

Having had a few weeks to think about this and talk with other developers, let me outline what I have heard as the desired features:

  • Some way of getting a promise for a module instance object by URL (call it window.importModule(url)), usable from classic scripts and module scripts.
  • Some way of getting a promise for a module instance object by specifier, usable only in module scripts, resolved relative to the current module script. (Call it import(specifier)).

Some people have asked for the following:

  • Some way of customizing the resolution hook, with fallback to the basic one
    • The most basic use case of this is just adding a bunch of mappings for "bare" module specifiers, with no resolving-module dependency. That is, everywhere in the app, "jquery" should point to the URL /js/libs/jquery.js. Everything that is not "jquery" should fall back to the default resolution algorithm.
    • A smaller number of people have asked for the ability to customize based on the resolving module as well, so that inside /node_modules/foo/foo.js, "jquery" points to /node_modules/foo/node_modules/jquery/jquery.js, but inside /node_modules/bar/bar.js, "jquery" points to /node_modules/bar/node_modules/jquery/jquery.js.

However, it's not clear that building in this customization is terribly advantageous, compared to just letting the service worker rewrite the module specifiers. Performance-wise, both are pretty bad; the preload scanner is entirely defeated, which is the biggest hit. I imagine in production people who are not bundling will use tools that rewrite their module import paths to absolute paths to allow the preload scanner to do its work. In any case, this kind of customization certainly seems lower priority.

I am not sure what to do in this area. But in general I am hopeful that trying to solve this problem is not seen as a blocker to solving the previous two.

I also got one ask for the following:

  • The ability to map specific URLs to specific source text

However this is clearly more in the territory of service worker.

Nobody has asked for the ability to reflectively construct modules and populate the module map through JavaScript, although some have wanted to be able to do that in the V8 API.

Nobody has asked for the ability to have customized loading behavior in different sections of an application.

@caridy
Copy link
Contributor Author

caridy commented Jul 18, 2016

@domenic this sounds good, I don't see any red-flags. The only detail that I'm not sure is about this:

Nobody has asked for the ability to reflectively construct modules and populate the module map through JavaScript, although some have wanted to be able to do that in the V8 API.

How are they suppose to interoperate with legacy module systems? If you import jquery, what does that means? is jquery an ES Module? is jquery a global value? is jquery a reflective module record? The only reason why the reflective module record exist is to provide interoperability with existing module systems, how are they going to survive without the ability to create a synthetic namespace exotic object?

@domenic
Copy link
Member

domenic commented Jul 18, 2016

@caridy jquery is an ES module, provided by a service worker or more commonly by ahead of time translation/wrapper modules. (That was the motivating use case behind the ability to map specific URLs to specific source text.)

From what I've found people are interested in having ES modules transparently solved in Node.js (through the V8 API), but in the browser they're already using bundling/transpilation/service worker rewrites, and turning to that to solve their problems.

@caridy
Copy link
Contributor Author

caridy commented Jul 18, 2016

jquery is an ES module, provided by a service worker or more commonly by ahead of time translation/wrapper modules. (That was the motivating use case behind the ability to map specific URLs to specific source text.)

jquery might be a wrong example here, but not everything can be a ES Module, the service worker, nor node, can make a script that runs in sloppy mode to be a module. Am I missing something here?

@domenic
Copy link
Member

domenic commented Jul 18, 2016

Right, in those kind of bridging legacy-compat cases people have suggested they would use a wrapper module. For example export default require("./sloppy-thing.js") with require operating on a browserify/webpack bundle, or export default window.myLibrary. But this was a pretty rare concern that people brushed past as a transitional thing; they seemed to think it was better to come up with such wrappers to help with the transition, instead of standardizing a runtime wrapper-generating mechanism.

That said this was rarely raised so maybe people just didn't think about it.

@martinheidegger
Copy link

I love many of the thoughts in here. Turning import statements into syntax sugar is particularly important. I think this would only be possible with tc39/ecma262#646 .

I wonder though if reusing import as a function (like import('x').then(...)) might be a bad idea.

  1. import is a reserved keyword in older javascript engines and thus could not be poly-filled.
  2. import foo from bar specifies to import foo from bar. while import() would need bar. Educationally that might be sort of backwards?!
  3. import('bar') would load the default export of bar How to load a baz which is exported in bar?

How about an API like:

  • importFrom('bar').then(...) delivering the default export
  • and for particular exports: importFrom('bar', ['baz', 'qux']).then(...)
  • for an import with named objects: importFrom('bar', {'a': 'baz', 'b': 'qux'}).then(...)

@matthewp
Copy link

@martinheidegger import('./foo.js') should load the module's namespace object, which includes .default and all other exported names. There was some talk about having a special dynamic loading function for getting the default export, but I don't know if such sugar is really needed since it's easily implemented in userland.

@caridy
Copy link
Contributor Author

caridy commented Jul 29, 2016

@martinheidegger @matthewp the decision this week was to spend time working with @benjamn on this matter, we had tremendous progress this week aligning behind a common goal, and hopefully we will have a formalization of the proposal for the next meeting.

@martinheidegger
Copy link

@matthewp A limitation on required exports is there to allow eventually dead-code elimination. Or garbage collection i.e. no module requests a particular export it could avoid executing a whole branch of code. You would lose this feature.

@matthewp
Copy link

matthewp commented Jul 29, 2016

You can't statically analyze dynamic loading code anyways. let exps = [ a ? 'a' : 'b' ]; importFrom('foo', exps)

Let's see what @benjamn and @caridy come up with, since it sounds like they are working on this subject now.

@martinheidegger
Copy link

@matthewp You don't need to statically analyze that. You could statically analyze the module and separate the branches that lead to the exports. Upon execution you could simply "not execute" the branch until its necessary.

@probins
Copy link
Contributor

probins commented Aug 3, 2016

Whilst I agree with the general thrust of this discussion that most apps probably don't need much in the way of custom loading, there's one thing I would like which hasn't been discussed here, which is the ability to pre-load dependencies - something like SystemJS's depcache.

In principle, HTTP/2 removes much of the need for bundling, but AFAICS the current loader is not taking advantage of this. It first has to fetch the 'main' module, find out what its dependencies are, then fetch those, find out what their dependencies are, and so on up the tree. Apps however know what dependencies they have, so ISTM it would be more efficient if the app could tell the loader what the dependency tree is before importing anything, so the loader can fetch/load the dependencies all at once and avoid all the round trips.

To some extent, you can do this in userland, in that apps could import all the dependencies in the main kick-off module script, but I'm thinking it would be better for this to be done with load() rather than import().

Has this been discussed anywhere?

@domenic
Copy link
Member

domenic commented Aug 3, 2016

In general that functionality is better served by higher-level technologies such as HTTP/2 Push or <link rel="preload">, and not something loader-specific. That is, it is a probably for any dependency graph, not just the JS module dependency graph.

@probins
Copy link
Contributor

probins commented Aug 3, 2016

the problem with server-side Push is that it doesn't take browser cache into account.

Preload links might be a good solution though, if they handle modules in a similar way to scripts. AIUI, rel="preload" separates fetching from execution, so in module terms the preload would be the equivalent of loader.load(), and execution would only occur when that module is imported. The HTML spec would have to be changed to allow as="module", right?

@domenic
Copy link
Member

domenic commented Aug 7, 2016

Some way of getting a promise for a module instance object by specifier, usable only in module scripts, resolved relative to the current module script. (Call it import(specifier)).

One thing that came up in discussions at the TC39 F2F with @bterlson is that there were use cases for allowing this inside classic scripts too. (I can't quite remember what they were?) I believe this is actually doable, spec/implementation-wise.

@Constellation
Copy link
Member

As mentioned in #37, we need to provide the way to share per-module-script-tree-scope information during loading to accommodate the whatwg <script type="module"> fetching semantics. :)

@mstade
Copy link

mstade commented Aug 16, 2016

I'm involved in building a web development platform with much of the common infrastructure you might expect, cdn, service hosting etc., but also development and runtime tooling such as compilers and loaders. My experience – working with tens of teams and a total of hundreds of developers, spanning over hundreds of web development projects using diverse technologies such as Angular, React, jQuery and most everything under the sun (sometimes all at once) – is that at production runtime nobody cares about anything but just loading things. A while back people were leveraging loader plugins (at the time requirejs) but over time this has kind of organically faded away in favor of build steps, which people have found easier to reason about.

At this point, I'd say that most modern (as in, recent) code we see uses declarative import/export statements, with any dynamic loading being quite rare and whenever it is used, it's vanilla with no esoteric use cases. The only case where this isn't true is when developers want to hot reload code, which pretty much amounts to a background process overwriting modules in the cache – that is, there's still no preprocessing going on at load time, it all happens in a background build.

For legacy code, our developers have been more than willing to develop wrappers that conform to modern standards rather than bending over backwards to make legacy work in a new world order. It's the pragmatic approach, more often than not requiring very little effort.

The tl;dr is that my anecdata is pretty much in line with what @domenic and @caridy are saying here and I as a platform builder very much welcome this direction.

hubot pushed a commit to WebKit/WebKit-http that referenced this issue Dec 8, 2016
https://bugs.webkit.org/show_bug.cgi?id=164861

Reviewed by Saam Barati.

Source/JavaScriptCore:

Originally, this "translate" phase was introduced to the module loader.
However, recent rework discussion[1] starts dropping this phase.
And this "translate" phase is meaningless in the browser side module loader
since this phase originally mimics the node.js's translation hook (like,
transpiling CoffeeScript source to JavaScript).

This "translate" phase is not necessary for the exposed HTML5
<script type="module"> tag right now. Once the module loader pipeline is
redefined and specified, we need to update the current loader anyway.
So dropping "translate" phase right now is OK.

This a bit simplifies the current module loader pipeline.

[1]: whatwg/loader#147

* builtins/ModuleLoaderPrototype.js:
(newRegistryEntry):
(fulfillFetch):
(requestFetch):
(requestInstantiate):
(provide):
(fulfillTranslate): Deleted.
(requestTranslate): Deleted.
* bytecode/BytecodeIntrinsicRegistry.cpp:
(JSC::BytecodeIntrinsicRegistry::BytecodeIntrinsicRegistry):
* jsc.cpp:
* runtime/JSGlobalObject.cpp:
* runtime/JSGlobalObject.h:
* runtime/JSModuleLoader.cpp:
(JSC::JSModuleLoader::translate): Deleted.
* runtime/JSModuleLoader.h:
* runtime/ModuleLoaderPrototype.cpp:
(JSC::moduleLoaderPrototypeInstantiate):
(JSC::moduleLoaderPrototypeTranslate): Deleted.

Source/WebCore:

* bindings/js/JSDOMWindowBase.cpp:
* bindings/js/JSWorkerGlobalScopeBase.cpp:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@209500 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@dschnare
Copy link

@mstade I agree whole heartedly with your suggestions, and as a platform developer myself I've come to the same conclusion. The ESModule loading mechanism should just load modules and not be concerned about build-time responsibilities at all (IMHO).

The rabbit hole never ends if you try to satisfy each and every possible and fantastical use case imaginable. If we try to do this, then we'll end up with this unnecessarily complicated it-can-load-anything-loader when what we should have been focusing on is producing a solid productivity platform for the future (i.e. easily groked and has a single responsibility).

For me and the teams I've been apart of, all we've ever needed was the ability to statically specify module dependencies via import/export semantics (or some other means like require/exports) and on the rare occasion be able to dynamically load a bundled/flattened module. Typically we choose a single module format, usually ESModule, and compiling to a format suitable for the target platform (i.e. CommonJS for Node, and either CommonJS or AMD for the browser). For anything that we need that doesn't follow our chosen module format we either run it through an automation script to convert it to the syntax we need or manually make the change ourselves. This has served me well so far, it helps to eliminate unnecessary loading and building complexities for those few modules/scripts that haven't been updated yet. And it's never a problem updating these scripts when a new version is released and we need to update because these scripts only have a few modifications to them and there are only every a handful of them to manage (if any at all).

However, having said that, what is becoming more of a need for large client-side apps is the ability to compile "dynamically linkable" bundles. Being able to do this with the -x and -r flags with Browserify optimizes build times if you use tooling like watchify to watch your source files. I could potentially see this being leveraged in production environments if it doesn't get complicated to deploy, but at this time I've only ever used it to optimize build times while developing and for production we have a separate build step to bundle everything in one bundle.

My two cents.

ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=164861

Reviewed by Saam Barati.

Source/JavaScriptCore:

Originally, this "translate" phase was introduced to the module loader.
However, recent rework discussion[1] starts dropping this phase.
And this "translate" phase is meaningless in the browser side module loader
since this phase originally mimics the node.js's translation hook (like,
transpiling CoffeeScript source to JavaScript).

This "translate" phase is not necessary for the exposed HTML5
<script type="module"> tag right now. Once the module loader pipeline is
redefined and specified, we need to update the current loader anyway.
So dropping "translate" phase right now is OK.

This a bit simplifies the current module loader pipeline.

[1]: whatwg/loader#147

* builtins/ModuleLoaderPrototype.js:
(newRegistryEntry):
(fulfillFetch):
(requestFetch):
(requestInstantiate):
(provide):
(fulfillTranslate): Deleted.
(requestTranslate): Deleted.
* bytecode/BytecodeIntrinsicRegistry.cpp:
(JSC::BytecodeIntrinsicRegistry::BytecodeIntrinsicRegistry):
* jsc.cpp:
* runtime/JSGlobalObject.cpp:
* runtime/JSGlobalObject.h:
* runtime/JSModuleLoader.cpp:
(JSC::JSModuleLoader::translate): Deleted.
* runtime/JSModuleLoader.h:
* runtime/ModuleLoaderPrototype.cpp:
(JSC::moduleLoaderPrototypeInstantiate):
(JSC::moduleLoaderPrototypeTranslate): Deleted.

Source/WebCore:

* bindings/js/JSDOMWindowBase.cpp:
* bindings/js/JSWorkerGlobalScopeBase.cpp:


Canonical link: https://commits.webkit.org/183164@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@209500 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests