jorendorff / js-loaders

Pseudoimplementation of the proposed ES6 module loaders.
54 stars 7 forks source link

Modular dynamic loading #89

Open guybedford opened 10 years ago

guybedford commented 10 years ago

To continue the discussion from https://github.com/jorendorff/js-loaders/issues/50

If I provide a module, that may dynamically load another part of itself later, I may want to write something like:

  export function create() {
    return System.import('./dynamic-dep');
  }

Relative dynamic loading, is only possible though if there is the ability for the module to know its own name so it can pass that to the import function (again assuming we normalize imports):

  export function create() {
    return System.import('./dynamic-dep', THIS_MODULE_NAME);
  }

Without this, dynamic loads within a package, are no longer possible to be truly modular.

This concept is used a lot in RequireJS.

I have no idea what the best implementation might be, but I do think it is an important use case.

matthewrobb commented 10 years ago

This doesn't seem like a good pattern considering every time you call create you would be overriding the module name in the registry.

However:

export default var module;

export function create() {
    return module = System.import("./dynamic-dep");
}

and

import { create, default as main } from "./above/thing";
guybedford commented 10 years ago

@matthewrobb if the import function runs normalization, and the module name was normalized relative to itself, the module would be defining into its own area of the registry it is allowed to change.

Say the main module is some/main/module

  System.import('./dynamic-dep', THIS_MODULE_NAME);  // THIS_MODULE_NAME = 'some/main/module'
  // ends up defining "some/main/dynamic-dep"
matthewrobb commented 10 years ago

@guybedford I guess my point is that either way it's a singleton.

guybedford commented 10 years ago

@matthewrobb The scenario I am imagining is not about the code structure, but rather imagine a date widget that can dynamically load its own internal module that handles date parsing, only when needed.

guybedford commented 10 years ago

@matthewrobb thanks for helping to clarify, always hard to communicate the exact idea.

jrburke commented 10 years ago

This can be solved by creating a module-specific System variable for each module. Then, any relative IDs passed to the System methods can be resolved relative to the module ID.

This would also allow for something like a System.module or System.self object that has the module-specific ID and URL on it, similar to the module variable in CommonJS/AMD (see related issue #53). This could also be where module-specific config could live.

The module-specific System variable would also hide away what actual module loader instance should be used for any dynamic loading within the module, since a module on its own does not have enough knowledge to know what module instance was used to load it, it just wants to use the same one for any dynamic loading.

guybedford commented 10 years ago

The best I can come up with for @jrburke's suggestion would require the following loader changes:

  // before evaluating "current_module"
  var scopedSystem = new Loader(System);
  scopedSystem.parentName = 'current_module';
  extend(scopedSystem, System); // copy paths config etc.
  global.System = scopedSystem;
  EvaluateModule('current_module', global);

An alternative to this would be to have the module name provided as a special import name, that is detected and populated when present:

current_module.js:

  import { name as relname } from '@module';

  export function dynamicLoad() {
    System.import('./local-module', relname).then(function(m) {
      // ...
    });
  }

The implementation of this one could then be based on System hooks only, instead of needing to modify the underlying Loader specification. So may end up being more sensible for an implementation.

A sample implementation might be:

System.normalize = function(name, parentName, parentAddress) {
  if (name == '@module')
    return '@module:' + parentName;
  return standardNormalize(name, parentName, parentAddress);
}
System.locate = function(load) {
  if (load.name.substr(0, 7) == '@module') {
    load.metadata.module = {
      name: load.name.substr(8)
      // could have other meta
    };
    return '';
  }
  return standardLocate(load);
}
System.fetch = function(load) {
  if (load.metadata.module)
    return '';
  return standardFetch(load);
}
System.instantiate = function(load) {
  if (load.metadata.module)
    return new Module(loader.metadata.module);
  return standardInstantiate(load);
}

It's still not as easy as it should be though.

Regardless, I can't stress enough how important normalization functions for import are.

guybedford commented 10 years ago

I very much think this last option would be the best way forward - moving the relative dynamic loading problem into the System loader, instead of the current spec.

If we can get normalization arguments for import, then we make massive strides to solving this problem.

parentName being null just means it is a top-level load, and therefore doesn't have a parent, and the name doesn't need to be normalized relative to a parent - basic normalization logic can handle this case very easily.

Then we can discuss a @module special case in the System loader when we get there.

guybedford commented 10 years ago

I've implemented a proof of concept here - https://github.com/guybedford/systemjs/commit/4183211adeba7518863731336931c90982d71320#diff-3ab20faf7299b13388c3f648a1d68ab0R1

The changes necessary are incredibly straightforward, exactly as described above (import to normalize with options.name and options.address, small System extension as provided above).

johnjbarton commented 10 years ago

Several points in the above confuse me:

Relative dynamic loading, is only possible though if there is the ability for the module to know its own name ... Is this correct? I mean the 'only' part? It seems to me that the compiler and runtime can conspire to ensure that Loader.import() knows the callers' module name. Maybe this is the essential character of @jrburke's proposal?

System.import to run normalization Is this even a point of contention? Of course it has to run normalization, otherwise every caller has to run normalization, which is craziness.

System.parentName What is the meaning of "parentName"? Based on the example, it looks like you meant System.referrer (or System.referrer if we must perpetuate misspelling).

I don't think there is any option wrt to dynamic import: the argument to dynamic import must work exactly like the argument to static import. Thus normalization of the user-given name must be performed relative to the caller as the referrer. Anything else is just a kludge.

guybedford commented 10 years ago

is only possible though if there is the ability for the module to know its own name ... Is this correct? I mean the 'only' part?

No, it's not correct. The two options are:

  1. Have the loader know the name internally already. This is as suggested by @jrburke and I then attempted to flesh this out after.
  2. Provide the name as a meta import, as I've provided above.

System.import to run normalization

Yes this is a point of contention. Currently, System.import does not call the normalize hook. Note this is different to static imports, which don't run through System.import. If I write System.import('jquery'), then the normalize loader hook won't be called, meaning I can't remap jQuery to a different canonical name. This basically stops any map configuration concepts from working, which are common in all loaders.

System.parentName

This is an attempt to create the internal context for (1) as in the first question above, and means the same as refererer. It is supposed to be the same as the parentName argument of the normalize function.

I don't think there is any option wrt to dynamic import: the argument to dynamic import must work exactly like the argument to static import. Thus normalization of the user-given name must be performed relative to the caller as the referrer. Anything else is just a kludge.

I think we agree? Currently normalize accepts an options argument with a metadata and address property allowed to be set. The address currently suggests the fetch address for the module. My proposal is to rather change this option to referer and accept the properties referer.name and referer.address for the normalize function, defaulting to null if not provided at all.

johnjbarton commented 10 years ago

Sorry, more confusion: System.import to run normalization While I was adamant above that import must normalize, how can the developer choose to provide a normalized name if the import() normalizes?

If import does not normalize, then the developer must normalize. If import does normalize, then the developer cannot normalize.

If import(name, options) included options.referrerName and import does normalize:

var normalizedName = this.normalize(name, options.referrerName || defaultReferrer, options.address);

then developer has all options (so to speak ;-).

I see that this is very close to your proposal. IMO the default for options.referrer (or options.referrer.name) has to be provided without assistance of the developer, to ensure that the static and dynamic cases work the same.

guybedford commented 10 years ago

I'm not sure how useful it is for a developer to provide a normalized name to import. Can you think of a use case for this, where having the ability to hook the name isn't needed at all? Note that names not starting with . are simply passed through as the default scenario.

If import does not normalize, then the developer must normalize. If import does normalize, then the developer cannot normalize.

Yes exactly.

Note that normalize can't just be called because it can also return a promise, so it's not a simple call either here.

If we really did need the ability to load from a normalized name, I would make this the responsibility of Loader.prototype.load since it does this already.

johnjbarton commented 10 years ago

First setting System.parentName, then calling Loader.import() seems like an error prone API. Wouldn't we be better served with an argument?

guybedford commented 10 years ago

@johnbarton Yes, the API for System.parentName isn't ideal, this is why the primary proposal is the following instead:

  System.import('./module-name', { name: 'parent/name' }) // load parent/module-name

Ideally the arguments would be System.import(name, [parentName,] [parentAddress]) to match normalization, but I've kept it as an options object to be as close to what we currently have in the spec as possible.

johnjbarton commented 10 years ago

Seems like

  System.import('./module-name', {referrerName: 'parent/name', referrerAddress: 'http://www.example.com'})

would be closer?

jrburke commented 10 years ago

I am getting a bit lost in the details of the plumbing, but want to stress the overall goal from my perspective:

No module should need to know its name or pass its name to loader API calls for those API calls to work. This is not how it works for the module syntax: the module does not have to get its name first for the syntax to work, and needing the name for System calls creates wordier APIs.

System as used by the module body should be a specialized object for each module that knows the module specifics. Maybe that specialized System object calls lower level loader APIs with the name, and maybe that is what you are discussing here, but want to be sure that is the case.

I just want to be sure that a module author does not need to grab the module's name before doing any System calls.

For cases where the module does want to know its ID (for other, non-System related uses, like for creating DOM ID prefixes or URL routes), then since the System object is specialized per module (hopefully), then that module information can hang off that specialized object. Uses of that data is more runtime in nature anyway, and it avoids needing extra text and a special @module to be available statically for module syntax. The syntax is wordier than just doing System.module.id, for example, it is not something that benefits from the static binding of the module syntax.

johnjbarton commented 10 years ago

@jrburke Since your comment is both reasonable and not related to my comments, I think we have a problem.

Sadly, the thing I call referrerName in my example above is not the name of the calling module. Should be, by name and logic, but it's not: I am highjacking this piece of the Loader system.

My goal is to load some modules from packageBaseAddress/moduleName.js. Here packageBaseAddress has a server part (baseURL) and a file system part, third_party/package say. I would like the source to be independent of versions and file-system changes. 'How to achieve this?

System.import('./moduleName', {referrer:'package'});

works if we separately map 'package' to 'baseURL/third_party/package' by configuration. The module names become package/moduleName and these map to baseURL/third_party/package/moduleName.js.

(My most immediate application is version related, my 'package' is traceur-version-n and it needs to co-exist with traceur-version-n+1 living at the same relative module names).

Am I on the wrong track?

guybedford commented 10 years ago

@johnjbarton yes these are completely difference scenarios. I think your scenario can be best solved by map and paths configuration.

Basically map configuration would involve something like:

  System.map['traceur'] = 'traceur@0.0.10/main';

This gets used in the normalize hook to convert traceur into traceur@0.0.10/main as the normalized name in the module registry and for the build.

To see an example implementation of map for the System loader, see https://github.com/guybedford/systemjs/blob/master/lib/system-map.js. This is a work in progress still.

Then assuming traceur@0.0.10/main is written with relative modules all the way, all the names will then turn into the desired normalized names wanted in the registry, all initiated from this first map value.

You can then use paths config to change the location of traceur@0.0.10, irrespective of what is in the registry. For example:

  System.paths['traceur@0.0.10'] = '/src/traceur/';

Or whatever suits here.

Between the two, you should have the full control you need here.

guybedford commented 10 years ago

@jrburke I understand your point here, but the best solution I could come up with for this based on the spec (https://github.com/jorendorff/js-loaders/issues/89#issuecomment-31646931), is not at all pretty, and I worry about the memory implications as well of duplicating an entire loader instance for each module.

I really don't see how writing:

import curModule from '@module';

export function dynamicLoad() {
  System.import('./local-module', curModule).then(function(m) {
    // ...
  });
}

is a problem at all, and at this point I think we need to focus on solutions that don't involve massive additions to the spec.

The only thing that is needed for my proposal to work is the ability for the System.import function to allow a refererName argument somehow.

In my opinion, getting this functionality in is the most important thing at this point!

That said I do welcome alternative proposals.

neonstalwart commented 10 years ago

@guybedford i think this misunderstanding of how @johnjbarton wanted to use this API shows why what @jrburke is suggesting might be a better option. if the context of the current module is somehow implicitly known to System then there's no confusion about (or unintended use of) an API where the module needs to be passed to it.

in my opinion the goal outlined by @jrburke provides a much more elegant solution from an end-user perspective and aligns with what CommonJS or AMD users would expect at this point.

johnjbarton commented 10 years ago

As far as I can tell the current js-loaders spec already requires:

import {x} from './y';
System.import('./y').then(function (m) {
  assert(x === m.x);
});

I support this and I believe that @guybedford does as well. As far as I can tell this @jrburke is proposing a specific way to accomplish this goal.

However, this goal is not enough, as far as I can tell. If you have a way to use this solution as a better option for the problem I outlined above, please let me know.

To clarify, the proposal I am discussing is to allow System.import() to have an optional argument to specify the normalize() arguments. The default values for these optional arguments would be as for the static import.

arv commented 10 years ago

Remember that the function does not know what context/module the caller is in.

System.import = function(name) {
  // There is no way to find out what the referrer should be here
  ...
};

This kind of different behavior depending on the call site has been shot down before in TC39.

@johnjbarton The example in https://github.com/jorendorff/js-loaders/issues/89#issuecomment-31886091 will only work if baseURI is the same directory as the current module.

jrburke commented 10 years ago

@guybedford while the existing spec may not allow for this, that spec is not done, nor is this repo example, and I believe the goal is to find areas where what has been drafted may fall short. I believe this is one of those areas.

A whole loader does not need to be cloned for each module, just an object that presents a System API that does not require passing module ID context to each call. That object could call the associated loader with the module with the correct module ID context underneath if it wanted to.

Note that System needs to be special for the module anyway because a module may want to dynamically load code that goes directly into the loader that loaded that module, not the global System, since that module could be loaded by a non-global System loader to start. So it is either specialize System or create some other variable that represents "loader that loaded me".

If that specialized variable exists, then it can also have the correct module ID context to handle the calls that need to know the current module ID for any relative ID resolution, as well as module-specific metadata, like the module's ID and URL.

IOW, the module loader needs to be able to handle the same kind of dynamic loading that is possible in AMD and CJS systems today:

// inside module 'a/b/c'
var dep = './d';

// in AMD:
require([dep], function (d) {
    //a/b/d is loaded in the same loader context as a/b/c,
    //which may not be the global/default loader.
});

// in CJS/node
require(dep);

// in ES?
System.import(dep, function (d) {});

@arv: hopefully the above clarifies the scope of the problem.

arv commented 10 years ago

@jburke Adding a binding to the scope of each module to its own System seems clean enough to me.

jorendorff commented 10 years ago

OK. To summarize this whole thread, the problem is:

This stinks. I think we should fix it.

Possible solutions proposed above:

  1. Make the import method call normalize, passing it options.name and options.address or something. Also provide a module-local binding, in each module, for the module's normalized name (like __name__ in Python).
  2. Like 1, but instead of having the user provide the referrerName and referrerAddress, infer it from context. This involves either stack inspection (which TC39 would shoot down) or different loader objects in every module.
  3. Give up. (Actually this was not proposed, but… well, now it has been.)

I like the first approach. It is simple and easy to use. The second would be even more convenient for users, but the improvement in convenience is slight, and the duplication of objects is kind of gross.

guybedford commented 10 years ago

It seems like this is being considered at the spec level.

If the module keyword is not being used any longer, my suggestion would be to use the module keyword for this information.