jorendorff / js-loaders

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

Run normalize for import #86

Closed guybedford closed 10 years ago

guybedford commented 10 years ago

It can be very useful to define a map configuration, along the lines of:

  System.map['jquery'] = 'jquery-1.8.3/jquery';
  System.import('jquery').then(function($) {
  });

map config is thus just like paths, except it happens before normalization. Note also that map['jquery'] is the same as adding both map['jquery/*'] and map['jquery'] in the paths analogy.

What was the design decision around not allowing import to run through normalize?

guybedford commented 10 years ago

Note having load not use normalize does make complete sense, and combined together with the above change, these two functions then offer flexibility for loading from a normalized or unnormalized name as necessary.

jorendorff commented 10 years ago

The design decision was like this: normalize relative to what?

Magically figuring it out via e.g. stack inspection is apparently a total nonstarter on TC39. I can see why. The same function should behave the same regardless of who's calling.

guybedford commented 10 years ago

The counter argument is that for the top level, normalize relative to an empty string or undefined makes complete sense (just design the normalize function to handle the empty parentName scenario).

Additionally, a second argument into import can be provided from metadata in the environment to allow flexibility.

johnjbarton commented 10 years ago

We are use normalize() for 1) canonicalization, 2) optional resolution relative to importer, 3) name mapping. If there is no importer value, the other steps are still vital.

Please see the bottom of issue #89 (we implemented __moduleName in traceur base on that post).

juandopazo commented 10 years ago

I was hoping to use normalize for conditional loading. If a certain test passes (like in yepnope's API), then I'd switch the name of the module for another one. But import not using normalize breaks this use case. Is the a chance to reopen this discussion? Or is there another way of doing name mapping?

unscriptable commented 10 years ago

Thanks @juandopazo! I was going to create a new github issue for this.

loader.import() is a serious abstraction leak. For a few reasons:

  1. As @johnjbarton said, the only place to perform mappings, aliases, etc. is in the normalize hook. Developers won't know that they're creating hard-to-find bugs when they fail to call normalize(). "Why do I have to call normalize() when I am not using a relative module name?"
  2. Devs shouldn't ever have to know that they're dealing with a loader unless they're actually manipulating a loader. Prior art shows this to be valuable: AMD's local require(id), CommonJS's "free" require(id) (and the proposed, but never agreed-on require.ensure()), as well as Java's Class.forName(String).
  3. All third-party code can't know which loader to use. They can't count on it being System and there's no way to reference the current loader.

There needs to be an equivalent to require(). Perhaps a function on the Reflect object that's been discussed? Just throwing this out there to see if it survives: Reflect.importDynamic(string): Promise :)

guybedford commented 10 years ago

I will do my best to answer these, to at least not leave them unanswered. Not sure if I am the best to be commenting though.

@juandopazo this is on my primary list of four features to push for the spec. The feedback I have got so far is that import should normalize. So I think we still have a good chance of getting this in.

@unscriptable the assumption from the spec, is that one would always use the System loader as the standard loading method. If you wanted a custom loader for a custom app that is fine, but System does become a universal global. This is why I created SystemJS as an extension library for System and not a separate global. The assumptions of the global System that would hold for any extension would be that it behaves like the spec system loader. So mutations would always add new behaviour, while not affecting the core behaviour of System. If a loader mutation was created to alter the core behaviour (such as blindly ignoring ./ in relative paths for example), then this wouldn't be a suitable mutation to use.

I hope that makes some sense in how one could have a spec-based System loader for its core test cases, with new cases that can be added.

juandopazo commented 10 years ago

@guybedford \o/ awesome! Let us know if you need us to add it to the agenda for the next TC39 meeting.

guybedford commented 10 years ago

@juandopazo yes it seemed promising from the informal feedback I got from Yehuda. It is still very much my personal wishlist though, and I'm not involved in the spec process, apart from simply providing feedback where I think it is necessary. I'm not sure how the module agendas are focused currently, but these points probably do need to start being discussed soon.

unscriptable commented 10 years ago

@guybedford ignoring for the moment that a global, mutable loader is the worst idea in the history of the web platform (and belongs in a different github issue)...

If we end up having a System.import(name) -- despite it unfortunately being the wrong abstraction -- then it should call the normalize hook. I agree.

guybedford commented 10 years ago

See https://bugs.ecmascript.org/show_bug.cgi?id=2659