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

Multiple instances of Loader #7

Closed
sheerun opened this issue Dec 22, 2014 · 36 comments
Closed

Multiple instances of Loader #7

sheerun opened this issue Dec 22, 2014 · 36 comments
Assignees

Comments

@sheerun
Copy link

sheerun commented Dec 22, 2014

Just heads up, it's important for some developers to be able to instantiate Loader multiple times:

systemjs/systemjs#60 (comment)

Please don't make System a singleton, but only a default instance. Also class-based approach is desired.

@sheerun sheerun changed the title Multiple insances of Loader Multiple instances of Loader Dec 22, 2014
@matthewp
Copy link

👍 Agreed, multiple loaders is very important.

@dead-claudia
Copy link

Maybe bringing back the Reflect.Loader constructor and System being only an instance, like how the API was at first in ES6 before getting pulled out into its own spec, would be the easiest way to resolve this.

@dherman
Copy link

dherman commented Feb 6, 2015

Multiple loaders could mean a few different things. Having separated out the Realm API from the Loader API makes it less clear to me how much of the functionality of multiple loaders should be achievable via new Realm and how much should be achievable via a Loader constructor.

For your use cases, do you only want different loading policies, but a shared registry between the two loaders? Or do you want to completely distinct registries? It seems reasonable to me to achieve distinct registries by creating new Realms, although maybe that's too hard to polyfill.

@guybedford
Copy link

The main use case for multiple loaders comes up when doing a build in Node. It is nice to not tie the build itself to the global environment it is running in, and be able to easily create and dispose loaders during the process. Being able to spin up a loading context without worry of clashes among other dependencies is very useful here. @matthewp can probably describe these use cases better as well.

@matthewp
Copy link

matthewp commented Feb 6, 2015

What @guybedford said, but also Loaders are useful whenever you need to load stuff that's not modules. For example Less.js has it's own loading pipeline which could be implemented as a custom Loader. This would make it more easily hackable. Eventually it would be interesting if HTML tags used custom Loaders to do the work. For example if all s went through window.imageLoader I could implement a translate hook that make them all greyscale.

@caridy
Copy link
Contributor

caridy commented Feb 6, 2015

I haven't seen a good use-case that requires sharing the registry. I think spinning up a new instance in a new realm could keep things easy and simple. We have a pretty basic (and small) Realm polyfills that we can probably adjust to support this today.

@matthewp
Copy link

matthewp commented Feb 6, 2015

This seems like it's going to make creating custom loaders much more difficult. If I'm a module loader author I now have to tell my users to create a new Realm? The class-based approach as @sheerun suggested seems natural in this case. It makes extending far more elegant than writing wrappers around a singleton. But I admittedly missed the conversations, why would a Loader be tied to a Realm anyways?

@johnjbarton
Copy link

Another reason for multiple loaders is testing. The test driver code needs to use a solid Loader while allowing mocks and fakes to be loaded for code being tested. Forcing test drivers to work through Realms would be a big barrier.

@dherman
Copy link

dherman commented Feb 7, 2015

After sleeping on it I don't really see why I was confused. While in the olden days the Loader constructor would give you a separate registry and a separate realm, it makes sense to think of a loader as encapsulating a registry but simply being tied to a realm. IOW the Realm constructor lets you create a realm and the Loader constructor lets you create a registry. This all seems like a pretty natural separation of concerns.

It also seems clear that fresh loaders should have distinct registries that start out empty. If you want to share between registries you can manually copy entries from one registry into another. (I guess one question would be whether there'd be use cases for actually keeping two registries in sync... maybe you could express that via the hooks?)

To sum up:

  • new realm: new Reflect.Realm
  • new registry: new Reflect.Loader
  • new realm and new registry: new (new Reflect.Realm).global.Reflect.Loader

Seem good?

Dave

@guybedford
Copy link

👍

@matthewp
Copy link

matthewp commented Feb 7, 2015

Sounds good! Will have to look it over when you update the spec. Being able to extend the default loader (whatever System is becoming) would be important as well. Assuming it's an instance of Reflect.Loader and we're not setting the hooks the old way I think it should work.

@caridy
Copy link
Contributor

caridy commented Feb 7, 2015

To sum up:

  • new realm: new Reflect.Realm
  • new registry: new Reflect.Loader
  • new realm and new registry: new (new Reflect.Realm).global.Reflect.Loader

👍

@matthewp
Copy link

@dherman I do have a use case in mind for keeping separate registries in sync. Doing this would require a way to iterate over the registry which I don't think is currently specced. I'll create a new issue to discuss this.

@nevir
Copy link

nevir commented Feb 18, 2015

(lurking) Is there still a default/global loader at some level? I.e., where do I go to install/lookup new entries into the default registry?

@guybedford
Copy link

To follow up on this - does this mean the class would set the initial hooks in the constructor itself:

class MyLoader extends System.constructor {
  constructor() {
    super();
    this.hook('fetch', function(url) {
      // ... custom fetch implementation ...
    });
  }
}

That seems to work out quite nicely I think, but just wanted to verify.

@dherman
Copy link

dherman commented Mar 11, 2015

@nevir Filed #34 to discuss that.

@guybedford TBH I haven't thought carefully enough about subclassing. I'm interested to collect constraints; perhaps @johnjbarton has some too based on what Traceur is doing. I assume you did System.constructor because you'd want to allow the default loader to be an instance of a host-dependent subclass of Loader?

@matthewp
Copy link

Related to this, why is hook a function and why does it exist? Currently it only sets the value on the loader. Were you thinking of having hook become more sophisticated than this? When I first saw it I thought maybe there could be a way to define a pipeline in this way. hook registers the function and all instantiates get called in order unless you do some type of preventDefault.

@dherman
Copy link

dherman commented Mar 11, 2015

Hm, I'm a little confused.

Related to this, why is hook a function and why does it exist? Currently it only sets the value on the loader.

Well, it both gets and sets each hook.

Were you thinking of having hook become more sophisticated than this? When I first saw it I thought maybe there could be a way to define a pipeline in this way.

It does define the pipeline, though, or at least it defines the extensible parts of the pipeline. It's how you can specify custom logic for hooking into the various steps of loading a module: how to fetch the payload, how/whether to translate the payload into source, and whether to pre-instantiate the module via custom logic.

hook registers the function and all instantiates get called in order unless you do some type of preventDefault.

I don't really understand this.

@matthewp
Copy link

Let me rephrase more clearly; what is the value in a function that simply gets/sets a property? Why would I want to use hook instead of

// Get
var translate = loader.translate;

// Set
loader.translate = myTranslate;

@dherman
Copy link

dherman commented Mar 11, 2015

@matthewp oic. It's not that important, except that since they are values-that-happen-to-be-functions, it avoids them looking like methods. Alternatively, it could be a sub-object, like:

var translate = loader.hooks.translate;
loader.hooks.translate = function(...) { ... translate(...) ... };

@matthewp
Copy link

Wait, is the value of this inside of the hooks no longer the loader instance?

@dherman
Copy link

dherman commented Mar 11, 2015

Not as the spec is written, no, but it could change. I'm guessing this touches on your expectations around subclassing. I think the question here is what is the most ergonomic way to make something where you can mutate the hooks but also subclass a loader class. And maybe what you're saying is that the most ergonomic way in JS for this is simply to make them inherited methods: you inherit default behavior from the prototype, you can override the default behavior in a subclass:

class MyLoader extends Reflect.Loader {
  fetch(key) { ... Reflect.Loader.prototype.fetch.call(this, key) ... }
}

and you can modify the behavior of an existing loader by putting a new method on the instance:

Reflect.Loader.current.fetch = function(key) {
  ... Reflect.Loader.prototype.fetch.call(this, key) ...
};

Is that your preference?

@dherman
Copy link

dherman commented Mar 11, 2015

I think the only thing that bothers me about this is that you have methods that indicate external-facing operations you can do on a loader (.import, .load, .resolve, etc), and the hooks represent another "namespace" of operations that are part of the internals. But maybe I should just get over it, since creating too many sub-objects or types risks getting overcomplicated. Or maybe we could just use naming conventions to distinguish them, like fetchHook, translateHook, instantiateHook or onfetch, ontranslate, oninstantiate.

@matthewp
Copy link

Yes, exactly, this is a perfect place to use subclassing.

But even if we didn't go this way you absolutely need to have access to the loader from within hooks. It's not uncommon to want to call other hooks (fetch is common) within a hook. And for complex extensions you need a place to store loader-specific data.

@matthewp
Copy link

Calling the hooks externally could be useful when you want the all of the extension behavior to be applied. Calling instantiate externally is probably never useful but the others can definitely be.

@dherman
Copy link

dherman commented Mar 11, 2015

But even if we didn't go this way you absolutely need to have access to the loader from within hooks.

Sure, but there are plenty of ways to have access to the loader via lexical scope. IINM the only question is whether you want to make use of inheritance e.g. to reuse a particular hook implementation without having to explicitly parameterize it over a loader; you get the loader for free via this. Which is fine, I think.

Calling the hooks externally could be useful when you want the all of the extension behavior to be applied.

Hm, maybe. I mean I could just not worry about it; there are only four hooks. But there are is one name collision (resolve) and one "conceptual" collision (load vs fetch). And still the purpose of these is as hooks, not as external methods. I think I'm more comfortable with a name convention. Is onfoo too cute? /me muses...

@matthewp
Copy link

Sure, but there are plenty of ways to have access to the loader via lexical scope.

This won't work if someone has call/apply the hook from a different loader.

IINM the only question is whether you want to make use of inheritance e.g. to reuse a particular hook implementation without having to explicitly parameterize it over a loader; you get the loader for free via this. Which is fine, I think.

I do think this is the best way. In the old spec I thought things were leaning this way.

@dherman dherman mentioned this issue Mar 12, 2015
2 tasks
@dherman
Copy link

dherman commented Mar 12, 2015

OK sounds good. I've moved subclassability to issue #35.

@guybedford
Copy link

@dherman about the naming conflict - surely the resolve hook is completely functionally identical in to the resolve method on the loader when it comes down to it? In this case perhaps the conflict doesn't exist?

Also surely load is never meant to be called by user code and is more an interface for script tags? Perhaps it's worth renaming these browser calls to something less likely to be used by a user?

@dherman
Copy link

dherman commented Mar 12, 2015

@guybedford Good point. We could just unify the two resolve methods and say that it's both a user-visible way to resolve module names using the current policy as well as the internal way of specifying the policy.

Also surely load is never meant to be called by user code and is more an interface for script tags?

Well it's also meant for frameworks that want to control when and how much individual modules are loaded, but it's true that it's very low-level. I need to think about alternate names.

@johnjbarton
Copy link

On the subject of multiple instance of the loader: in experimenting with new System.constructor() I find it awkward for nodejs use. It seems like the baseURL should be the directory of the module that calls new System.constructor(), but my simple implementation gives the same baseURL to the secondary loader.

Will the spec say anything about this issue?

@guybedford
Copy link

@johnjbarton there was a baseURL discussion in #23 if that helps.

@caridy
Copy link
Contributor

caridy commented Aug 21, 2015

Solved by #65

@caridy
Copy link
Contributor

caridy commented Aug 21, 2015

subclassing is now supported via:

class MyLoader extends Reflect.Loader {}

hooks are now defined thru symbols:

System.loader[Reflect.Loader.resolve] = function (name, referrer) {
   // implement your custom resolve on the default loader
};

// or
let myLoader = new MyLoader();
myLoader[Reflect.Loader.resolve] = function (name, referrer) {
   // implement resolve on your custom loader instance
};

@caridy caridy closed this as completed Aug 21, 2015
@matthewp
Copy link

👍

I'm assuming

class MyLoader extends System.loader.constructor {}

Should work as well, to extend what System.loader provides in addition to the base Loader?

@caridy
Copy link
Contributor

caridy commented Aug 21, 2015

@matthewp yes! at this point we don't have a strong reason to prevent that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants