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

0.50: Initial review of loader.js #500

Closed
wants to merge 1 commit into from
Closed

0.50: Initial review of loader.js #500

wants to merge 1 commit into from

Conversation

probins
Copy link
Contributor

@probins probins commented Jun 22, 2016

Nothing against Joel's work, of course, but I'm seriously wondering whether it wouldn't be simpler to start from scratch. I hadn't realised that the spec had changed so much.

@probins probins changed the title Initial review of loader.js 0.50: Initial review of loader.js Jun 22, 2016
@@ -73,7 +81,7 @@
});

else if (stage == 'link')
return requestLink(loader, key, metadata).then(function() {})
return requestLink(loader, key, metadata).then(function() {});
Copy link
Member

Choose a reason for hiding this comment

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

We do need this semicolon?

@guybedford
Copy link
Member

I'd rather not just remove this code actually. Better to get the tests running then move through code changes that keep the tests running. Otherwise the codebase will drift off too much - it then becomes as good as a rewrite getting the tests going eventually.

@guybedford
Copy link
Member

(Happy to take comment PRs that update the function numbers, note spec deprecations though)

@probins
Copy link
Contributor Author

probins commented Jun 27, 2016

although in principle I think it would be better to start from scratch, in practice I'm not sure that will work, as I doubt whether I will have time to do more than a small part of what needs doing. I can do some work over the next couple of weeks, but after that I'm not sure.

So an incremental approach might be better, but I'm not sure how that would work either, as changing for example Loader.prototype functions means you have to implement the new functions called by those functions, which for the most part don't currently exist or need rewriting. The Registry functions should be reasonably straightforward to implement, so perhaps we could start with that.

But to get the tests working, the environment needs changing - later version of Node, include URL in Node version including Travis, module-only version of Traceur/Babel (what about Typescript?), catering for script type="module" being implemented in browsers. So maybe we should start with that, and leave loader.js for the moment.

As for the missing semicolon, this is something the linter in my editor complained about. I think it would be better for Travis/the build system to run a linter, but presumably it doesn't atm because of the way the code is split into different files and concated into a wrapper. So perhaps converting to ES6 modules and using Rollup to convert into IIFE should also be a priority?

For the time being, I'll change this PR to be comments only. I'll list in the comments for each function which other functions it runs/is dependent on.

I'll also try and draw up some task lists. Rather than preface all these with '0.50', it might be better to create a new Github label and/or milestone for this. I could do this, but because of Github's limited permissions system would have to have commit rights to the repo - otherwise you would have to do it.

@guybedford
Copy link
Member

@probins whatwg/loader#147 changes things significantly now so I think we should put this effort on hold until that clarifies. It does sound like that will be largely a rewrite direction, so I will aim to jump into this work as soon as it becomes possible now.

@probins
Copy link
Contributor Author

probins commented Jul 1, 2016

yes, I noticed that, though it's all rather vague atm "not an actual plan, just food for thoughts"

@probins
Copy link
Contributor Author

probins commented Jul 1, 2016

might as well close this for the moment

@probins probins closed this Jul 1, 2016
@guybedford
Copy link
Member

Now to plan that upgrade path :)

@probins
Copy link
Contributor Author

probins commented Jul 9, 2016

whatwg/loader#147 changes things significantly now

@guybedford I'm not sure it does, does it? It changes what is exposed to users, but AIUI the basic Reflect.Loader and Reflect.Module are unchanged. The default browser loader too remains as is, just that it wouldn't be exposed as System.loader any more. The new import() would be used in apps instead of System.loader.import, and you could access the current loader with import.loader. IOW, we can carry on with changing Reflect.Loader and Reflect.Module, leaving the System side of things till later.

@guybedford
Copy link
Member

@probins if you do want to continue working on PRs based on the new directions I guess we can do that too.

@probins
Copy link
Contributor Author

probins commented Jul 25, 2016

shan't have time this month; perhaps in August. So if you have time to work on this ...

@guybedford
Copy link
Member

I've been working on a rewrite already of just the shell API for a fast
production loader. Will share something on an experimental branch soon and
copy you in.
On Mon, 25 Jul 2016 at 17:50, Peter Robins [email protected] wrote:

shan't have time this month; perhaps in August. So if you have time to
work on this ...


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#500 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAkiyvcSg83E2qG53ZjKW2jGxG2EL8CDks5qZNtGgaJpZM4I76Re
.

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.

2 participants