Release new loader branch

[guybedford]

It's really important now to release the 1.0 branch so that this project can again match the loader spec more closely.

I think it would make sense to keep it as a minor patch - say 0.20, at least until the loader spec is stable.

Even if we're not fully spec compliant, having the spec API is obviously a much better thing than still being on the old API, and it's a shame the new loader branch has just sat there now for over a year.

If anyone is interested in working on this please get in touch. If we can release it tomorrow we should. I'd hate to be holding this work back.

[probins]

what still needs doing? This is presumably linked with #381. I agree that it's high time this project moved on from the old spec. Would it help if we created a milestone with the outstanding tasks listed with 'PR welcome' or some such?

[guybedford]

I don't think we're even linked to 381 at all here either, as this project can continue to make regular breaking changes due to its nature.

We're currently running on an older version of the loader spec, so we could update to the latest PRs made over the last year, or at least a set of them including things like ensuring the new globals are all correct.

Then the main thing secondly would be a Babel 6 upgrade / recent Traceur upgrade, and updating the docs.

[probins]

and an update to SystemJS :-)

It's a long time since I looked at the 1.0 branch but I have some time, so, if you like, I can look through the code and see what I think still needs doing. Probably won't have time for any meaningful testing, but if it's usable it could be released as alpha/beta. I can also see what state the docs are in.

[guybedford]

In due course SystemJS as well certainly, but I think we should get this update out independent to SystemJS, and SystemJS can upgrade when the timing makes sense.

If you're able to look into this that would be a huge help!! As I say the rough main things are probably checking the top-level global names, Babel / Traceur upgrades and the docs. I can certainly provide as much detailed feedback as is necessary too.

[probins]

checking the top-level global names

you're meaning here Reflect.Loader and System.loader? Are there any other globals?

[probins]

Babel / Traceur upgrades

the question to me is what we're doing with transpilers - as in discussion of #463.

[probins]

ok, have now looked through the code. The bad news is that there's quite a few functions (in lib/loader.js) that do not match the current spec. The good news is that they're well documented with 'TODO', and from my brief look at least some of them should require little change. However, I don't think this is something that can be implemented 'tomorrow'.

On the top-level globals, lib/wrapper-end.js exports Reflect.Loader, Reflect.Module, and Reflect.global (is this official?). The system loader however is being assigned directly to System, whereas it should be in System.loader. I can submit a PR to change this.

I notice a couple of other places where out-of-date functions are being used:

• declarative.js uses new Module() which I think should be new Reflect.Module()
• module-tag.js uses System.provide() - not sure what this should be

system.js is creating site, though this is not (yet) in the spec.

On module-tag.js, as implementations are starting to appear, I think there needs to be some way for this not to execute if natively supported; trouble is I have no idea how you check that :-(

I would move the URL polyfill into userland. This is only needed for IE, and IMHO it should be up to the app developer to decide whether they want to support IE or not.

[probins]

one other out-of-date function: system.js is using hook(), which is still in loader.js but is no longer in the spec. This should be changed to the appropriate hook function, resolve() etc.

[probins]

@guybedford here's what I propose doing:

• create a new branch based on 1.0; I'd suggest calling this 0.50 rather than 0.20 to make clear this is a breaking change
• tidy up the out-of-date functions listed above
• start not with the loader but with the docs:
  • change the target browsers to modern ones, all of which now support ES6, including Map and Promise, as well as URL() (Safari is lagging a bit atm on general ES6 support, but this will change with the next release, due I believe in September)
  • older browsers, such as IE, can be used providing any ES6 code is first transpiled to ES5, and polyfills for Map, Promise, and URL are loaded by the app before the ModuleLoader, for example, from cdn.polyfill.io
  • remove URL and Map polyfills from code
  • support for ES6 module format:
    • native support if available (not atm sure how you check for that)
    • on-the-fly transpilation using traceur/babel if no native support (not suitable for production) (this implies using a build supporting only import/export)
    • support for System.register available with SystemJS
  • no longer support for paths, but this is available with SystemJS
  • list api changes to loader:
    • System.import becomes System.loader.import (apps can supply alias to ease transition)
    • other changes really only needed by those with custom loaders
  • clarify that module 'names' are now URLs (the spec is vague on this subject, but URLs are all that script type="module" implementations will support)
• as a start on loader.js, rearrange the current code to correspond with the spec, removing obsolete functions, and creating empty functions if no code currently available

We'll have to see then what time I have available for filling in the empty functions.
The tests also need to be changed/brought up to date.

[guybedford]

This all sounds really great, it definitely sounds sensible to take a high-level docs focus first. I've included some feedback below.

create a new branch based on 1.0; I'd suggest calling this 0.50 rather than 0.20 to make clear this is a breaking change

Sure, I've created a 0.50 branch based on the 1.0 branch.

change the target browsers to modern ones, all of which now support ES6, including Map and Promise, as well as URL() (Safari is lagging a bit atm on general ES6 support, but this will change with the next release, due I believe in September)

Agreed!

no longer support for paths, but this is available with SystemJS

Yes happy to remove the sites implementation.

as a start on loader.js, rearrange the current code to correspond with the spec, removing obsolete functions, and creating empty functions if no code currently available

This code was exactly to the spec previously, so following spec commits since the date of the loader code may be one way of going about this upgrade. This is a tough job though, so I certainly understand if you don't have time for this. I think a documentation-first focus and getting the other code points you've mentioned right first makes sense here. If we have something that runs, updating this project to that as an alpha with a caveat that this is still being updated to the new spec would be a great start, as it means people can be testing out the module tag, or testing out the loader spec API and seeing how it fits together, which should be the whole point of this project - to show real working loader code, and to help demystify the spec.

[probins]

people can be testing out the module tag

this is something that bothers me. I was expecting Chrome to provide the first implementation, but Edge seems to have got there first https://blogs.windows.com/msedgedev/2016/05/17/es6-modules-and-beyond/. If a user has this enabled in the browser, it will process any module tag(s), and in this case the polyfill should not do anything with the tags. But I can't think of any way for the polyfill to know whether this has been done or not. Any ideas?

[probins]

also related to module tag, https://html.spec.whatwg.org/multipage/webappapis.html#integration-with-the-javascript-module-system only allows /, ./ and ../ in module imports, relative to the calling module's base URL. For compatibility, should the polyfill require this too?

[guybedford]

@probins definitely a feature detection can be put together for existing implementations of the module tag. Eg creating a module tag and injecting it into the head it see if it executes.

Yes the module tag restricts names to URL-like. The default loader resolver would implement the same algorithm also throwing on plain names, but then resolver extensions would be able to remove this restriction, as it is exactly designed to be overridden by the ability to set a custom resolver and that is why it is there.

[probins]

injecting it into the head it see if it executes

yes, I suppose it could create a property in System, for example, and then check whether it's been created or not.

ok, I'll start work on a Readme rewrite this week some time. I'll create small PRs rather than one large one.

[probins]

remove URL polyfill from code

one thing I was forgetting was that Node does not have URL(), so some sort of module/polyfill is needed for running in Node - which includes running Travis/tests

[probins]

something else to decide is what to do about metadata. The current loader includes metadata as a param in all the prototype methods, but this is not (atm) defined in the current spec. Remove this for the time being?

[guybedford]

one thing I was forgetting was that Node does not have URL(), so some sort of module/polyfill is needed for running in Node - which includes running Travis/tests

This would be an argument for bringing back the internal URL polyfill actually. No reason not to support Node and IE out the box without further configuration.

something else to decide is what to do about metadata. The current loader includes metadata as a param in all the prototype methods, but this is not (atm) defined in the current spec. Remove this for the time being?

Sure.

[probins]

This would be an argument for bringing back the internal URL polyfill actually. No reason not to support Node and IE out the box without further configuration.

have a separate build for Node which includes the URL polyfill? When a function is only supported in a minority of browsers, then I think it's fair enough to always include a polyfill, but according to http://gs.statcounter.com/ IE usage is now down below 10% (if you count desktop+mobile). Even allowing for those using older versions of other browsers, that's still only a small proportion, so you're adding unneeded overhead for the majority just to cater for the minority.

[guybedford]

The URL polyfill still has spotty implementation consistency as well... I think we should just blanket keep it in actually. Evergreen should be the target... which means supporting all modern browsers out the box without any extra configuration necessary.

[probins]

@guybedford a question: why does loader.js currently include functionality for transpile/System.register? Is this not something for translate()?

[guybedford]

@probins this is in order to actually be able to run the native linking code. If we implement this in instantiate / translate then a whole bulk of the code in the spec becomes useless.

Theres two approaches here:

1. Just don't implement it at all as the nature of this polyfill.
2. Implement it in just a dev build of the loader, and have a separate production build that then skips out the native ES module linking.

(2) is the approach we've taken here so far.

[probins]

on hold pending whatwg/loader#147

[guybedford]

@probins if you're interested to review, I've just posted up a new loader rewrite on the 1.0 branch!

[probins]

:-) I'll take a look in the next few days

[probins]

I really like the change to modules/rollup - much cleaner and easier to read. Also like the System.register build, so I built this. But when I run test.js as suggested, it fails 'loader is not defined'. Not immediately obvious to me where it's failing.

[probins]

I then tried a similar test in the browser:
demo.html:

<!doctype html>
<html>
<head>
</head>
<body>
  <script src="dist/system-register-only.js"></script>
  <script>
var loader = new SystemRegisterLoader(document.baseURI);
loader.import('./test/register.js').then(function(m) {
  console.log(m);
})
.catch(function(err) {
  console.error(err);
});
  </script>
</body>
</html>

This fails completed is not defined, which is correct - it isn't :-)
module-scripts.js l5 /completed/ready/
Run again, correctly logs the ModuleNamespace

I'll try some further tests with System.register in the browser, which is my main interest; ATM I'm not too bothered whether it works in Node or not. AIUI, System.register should also work in <script type="module"> - is that correct?

[probins]

a couple of other things my editor complains about:

• common.js l53 __global not defined - envGlobal?
• fetch.js l94 isWindows not defined; also the 'fetch' at the end refers to r which also doesn't exist, should presumably be res; and why not use catch here?
• resolve.js l7 isNode and l22 throwResolveError not defined

Also, in system-register-only.js, baseURI is imported but not used.

[guybedford]

@probins thanks for these I've added the changes.

[probins]

test now works in Node and my demo.html, thanks. I should be able to find time tomorrow to go through the loader in more detail; won't be anything like a test, more a comparison with the spec.

[probins]

ok, Guy, here's some preliminary comments:

• I think the intro stuff in 0.5 readme should be copied over; apart from that, 0.5 can probably be deleted
• In general, I agree that the polyfill doesn't need to implement the whole spec, just those bits affecting the public API, but I think the API/parameter diffs from the spec and from the previous version should be highlighted in the readme. These were the ones I spotted:
  • a couple of minor ones first: no realm; no error/sanity checks
  • Loader/Module not added to Reflect
  • 3.1.1 Loader() baseKey - what's the reason for this?
  • 3.3.4 load() missing stage
  • no 3.3.5 get loader registry - this would be helpful for examining in console, if nothing else Ignore! Can look at this directly in the loader instance
  • deps handling in custom loader; ensureEvaluated() also in custom loader
  • having to instantiate loader in apps - no default loader - though can do something like:

  <script src="dist/loader-system-register.js"
    onload="window.System=window.System||{};System.loader=new LoaderSystemRegister(document.baseURI)">
  </script>
  <script type="module" src="./myBootstrap.js"></script>

What's your plan? Presumably you'll be adding more tests. I have been planning on converting my current modules, still in CJS, to ES6/System.register, and could then use this register build to load. This would be a more substantial test; however, I don't use much of the loader (just import), and anyway conversion won't happen overnight.

[probins]

might also be worth stressing that System.register now works in <script type="module">, which I don't think it did before

[guybedford]

@probins thanks for the further feedback, I've included some notes in the spec differences section. I've restructured the project some more, so the direction should be clearer now... I'd value your opinion if the general structure makes sense to you? (having built and published the three example loaders now, this is now the base for the next version of SystemJS)

[guybedford]

I've published this now to https://www.npmjs.com/package/es-module-loader. @Rich-Harris kindly offered the es-module-loader name on npm, so it makes for a smoother transition than having to release a 1.0 on the es6-module-loader package.

I think the main thing to work towards is just getting the documentation, API documentation and tests to a point where this can be exposed as the face of this project on GitHub and shared. I'll be working towards this tomorrow and Friday, hopefully to get something out this week!

[probins]

in general, I like the new structure, but system-register should include support for <script type="module"> or at any rate it should be easy for me to include it in a custom build. I'm preparing for its implementation, and the advantage of using something like what I posted yesterday is that I can use polyfill with that, and then when it's implemented simply remove the polyfill load for use with raw ES6. loader.import() can be replaced with a function that dynamically creates module script tags.

[probins]

on docs, there's quite a bit of relevant stuff in systemjs/systemjs#1143 which could be either split or just provide a link from here to there.

[guybedford]

@probins it would be nice to support module scripts for System.register, but the problem with this approach is that it will error when browsers do support <script type="module"> .

[probins]

there has to be a userland check anyway (we had a discussion before about how to implement that): if supported load ES6 else load polyfill with System.register. That's the only way I can see to handle the transition when only some browsers support.

[probins]

to expand a bit on my previous code: if ES6 supported, create module script to load myES6Bootstrap.js else create scripts to do:

  <script src="dist/loader-system-register.js"
    onload="window.System=window.System||{};System.loader=new LoaderSystemRegister(document.baseURI)">
  </script>
  <script type="module" src="./myRegisterBootstrap.js"></script>

[guybedford]

It certainly can work for content sniffing the response to myRegisterBootstrap.js, but we can't assume content sniffing necessarily.

[probins]

in fact, this isn't necessary, as I can just as easily use import() in the polyfill onload - and that has the advantage of making the build a bit smaller.

What do you see as the advantage of using System.register in Node, as opposed to converting ES6 to CJS and using the existing loading mechanism?

[guybedford]

Ok, I've launched this now as a 1.0 release.

I've been doing some performance benchmarking and it looks like this new loader is on average 10 - 100x faster at loading bundles than SystemJS! Now to get SystemJS updated to this base...

@probins System.register is all about replicating the exact execution semantics... in future this will include the loader context (import('./x'), import.key) and also top-level await support (which is simple to add to System.register, not so simple to add to commonjs!).

[probins]

great stuff - a giant step forward! 👏 I'll go through the docs at some point soon.

If SystemJS is converted to a set of ES6 modules too with an uglified System.register version in dist/, it should be easy to add them singly in the browser as required rather than having to go through a separate build step.