Skip to content
This repository was archived by the owner on Jul 13, 2020. It is now read-only.

1.0 Spec Rewrite - Work in Progress - #317

Closed
guybedford wants to merge 8 commits into
masterfrom
1.0
Closed

1.0 Spec Rewrite - Work in Progress#317
guybedford wants to merge 8 commits into
masterfrom
1.0

Conversation

@guybedford

Copy link
Copy Markdown
Member

No description provided.

@guybedford

Copy link
Copy Markdown
Member Author

Note it's way easier to review this in the branch - https://github.com/ModuleLoader/es6-module-loader/tree/1.0/src

@probins

probins commented Feb 27, 2015

Copy link
Copy Markdown
Contributor

a major change! You're calling this 1.0 - does this mean you regard the spec as stable? I see quite a lot of Todos in it, which implies that there's still quite a lot to do before it's finished.

Does the change from module names to urls/addresses mean that System.paths is no longer needed?

@probins

probins commented Feb 27, 2015

Copy link
Copy Markdown
Contributor

I'd suggest adding a comment to system.js that this corresponds to section 8 of the spec

@guybedford

Copy link
Copy Markdown
Member Author

The release plan is to start with an alpha, then to release a series of betas, before the 1.0.0 stable. These prerelease phases will happen over the coming months. Then there's no rush to the 1.0.0. The spec API is actually pretty small so that there isn't a high risk in a 1.0.0 at all. We'll then allow for new major versions as corrections to the spec, aiming to reach stability before 10.

@probins

probins commented Feb 27, 2015

Copy link
Copy Markdown
Contributor

Does the change from module names to urls/addresses mean that System.paths is no longer needed?

looks like System.paths is replaced by site.mappings. The docs will have to be changed to reflect this. Not sure how you do that with the wiki, as wiki is not version-specific. AIUI, module-tag.js and parser.js are optional, and can be left out if you're not loading ES6 modules. If that's correct, this should also be documented in the Readme.

@guybedford

Copy link
Copy Markdown
Member Author

Yeah that's an issue with using the Wiki for docs. Once the change is through though shouldn't be as much of an issue - we can keep all the existing info in a legacy documentation somewhere.

module-tag.js and parser.js can be removed but it's only a 1KB saving, and for simplicity of use I think it's worth just leaving them in. It retains the full polyfill meaning with expected results that way.

@guybedford
guybedford force-pushed the 1.0 branch 5 times, most recently from 888f2bc to a65478f Compare February 27, 2015 17:07
Comment thread src/parser.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this intentional having 'm .' rather than 'm.' throughout this function?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look. Yes it is because I want the loader itself loadable through SystemJS, which detects the register format in code as the existence of the "System.register" string in source, as the highest priority format.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is only being used as a lookup, right? If so should really be an object instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Yes that would be a great point for perf critical, but this function is only called a handful of times.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really all that hook does? This is disappointing. I don't see the point, actually. I thought/hoped hook somehow managed having multiple functions for the same hook.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is what is currently defined. Do share your feedback on the spec repo if appropriate.

@guybedford

Copy link
Copy Markdown
Member Author

Updated to whatwg/loader@2946478.

NB still need to port from 69b2589 on master.

@guybedford
guybedford force-pushed the 1.0 branch 2 times, most recently from a34508c to fcb809a Compare April 2, 2015 22:15
@guybedford

Copy link
Copy Markdown
Member Author

The branches have now diverged, so closing this PR.

The 1.0 branch will be continually actively developed now.

@guybedford guybedford closed this Apr 3, 2015
@probins

probins commented Apr 3, 2015

Copy link
Copy Markdown
Contributor

how usable do you regard 1.0 branch? Should I switch over to using it now, or would it be better to hold off for a while?

@guybedford

Copy link
Copy Markdown
Member Author

It would be great to get feedback, but it's just for experimentation still. I'll aim to get an alpha out as soon as it makes sense. Even then no rush to upgrade.

@probins

probins commented Apr 3, 2015

Copy link
Copy Markdown
Contributor

ok, I'll try some 'experimentation' :-)

@matthewp

matthewp commented Apr 3, 2015

Copy link
Copy Markdown
Contributor

@guybedford I still don't understand why there is a System. I know it sounds pedantic but there is no System any more, in the new spec. SystemJS can still alias window.System to Reflect.loader so there's no breakage. I guess I just don't understand why this project needs to have non-spec properties. Or is this just a temporary measure because you were more focused on implementing the important parts of the new spec?

@guybedford

Copy link
Copy Markdown
Member Author

@matthewp good question - Reflect.loader doesn't seem that set yet. These important points do still seem unclear so these are the sorts of things we're waiting on before an alpha.

@matthewp

matthewp commented Apr 3, 2015

Copy link
Copy Markdown
Contributor

Ok, I understand that perspective. So far you've been focusing on the big parts that are unlikely to change (unless I have my way ;) ), so stuff like naming can be iterated on. Fair enough.

@probins

probins commented Apr 3, 2015

Copy link
Copy Markdown
Contributor

AFAICS Reflect.loader (as opposed to Reflect.Loader) isn't in the current spec either

@matthewp

matthewp commented Apr 3, 2015

Copy link
Copy Markdown
Contributor

The spec says Reflect.Loader but in the issues its been agreed that Reflect.Loader will be the constructor (there isn't one in the spec now) and something else, probably Reflect.loader will be the default instance. But what isn't in the spec at all, is the word System. :-)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants