Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement module reload extra #2014

Closed
wants to merge 3 commits into from

Conversation

LarsDenBakker
Copy link
Contributor

Fixes #1971

This implements a reload extra, which enables hot module reloading/replacement. This is my first time doing something on SystemJS, let me know if there are things to improve.

I've tried to cover the possible race conditions and error scenarios I could think of, but I probably missed some.

I added an extra argument to getOrCreateLoad and import to allow passing along the importerSetters from the previous instantiation of the module. This way the live bindings can be updated with very little extra code.

The extras don't have access to the load wrapper object, I added a method in the registry feature but this might need to be solved in a different way.

Setting window.TRACING outside tests isn't a nice thing to do, maybe it should be a setting on the SystemJS namespace instead?

@LarsDenBakker
Copy link
Contributor Author

LarsDenBakker commented Sep 1, 2019

While trying this out in a real project, I'm running into an issue where if a dependency fails to load re-importing will keep trying to run the failed module as well. This is because I re-import the module that's being reloaded in order to make sure it's fully finished loading, running top level await etc.

Maybe it's better to not re-import, but just to await the specific promises on the load wrapper. I wasn't sure which one I should wait on though, any advice?

Copy link
Member

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

I have been waiting so long for someone to tackle this. @LarsDenBakker I can't think of a better person to do this, thank you so much for diving in!

I've left some comments re some changes to the API. Will do my best to work through the considerations promptly here to get this merged, and please don't let my reviews discourage progress.

The hot reloading API you have here is a really elegant and fast reload, and I think it will create an excellent user experience.

RollupJS are very keen to be able to support hot-reloading too, so I look forward to building those workflows out here too.

In designing hot reloading it is worth keeping in mind some flexibility of the approach, and to keep the following at the back of your mind -

  1. Parent Reloading: Parent modules can perform operations on child modules that put them into specific states. Eg consider running mocha.setup({ ui: 'tdd' }) or applying an Acorn plugin. Therefore reloading the child will clear that state, and the parent module won't necessarily know to reapply it. In these cases it can be preferable to entirely clear and reload all parents of the module as the easiest way to recreate this state, and providing this as eg a boolean option to the hot reload function could be useful.
  2. Reload events: If we wanted to support parents reapplying state without needing to be reloaded, it is often useful to support reload event systems like Webpack does. There are many ways to skin this one. Parents can be notified of child updates. Children can be notified of their own dehydration and rehydration. But usually any hot reloading model will want to include these sorts of things at some point. I would personally be for some import.meta.onhotreload = function () {} style things here, which should be possible under the current hooks model. We don't need to set these APIs up now, but bear them in mind as you work on this problem as they will likely be the next step once we have these core primitives.

While trying this out in a real project, I'm running into an issue where if a dependency fails to load re-importing will keep trying to run the failed module as well.

Can you clarify this case? It might actually be a core bug.

Say I have:

import './dep-with-error.js';
console.log('hot reload');

where dep-with-error contains:

throw new Error('error');

then we would expect that hot reloading the first module would result in seeing the hot reload message twice, but we should only see the dependency error once because execution errors are cached in the registry unless explicitly cleared by a loader.delete operation.

If this is not the case then that is a bug we can fix certainly.

Fetch errors are another question - in the browser they are currently looking at making fetch errors retry to support nicer behaviours.

@LarsDenBakker
Copy link
Contributor Author

LarsDenBakker commented Sep 1, 2019

Thanks for the review. And no problem, I expected that this will be some back and forth. It's quite a complex feature.

@LarsDenBakker
Copy link
Contributor Author

I've rebased on #2020 and updated the reload code. I still have some questions regarding the reload update in the other PR.

@LarsDenBakker LarsDenBakker force-pushed the reload branch 2 times, most recently from 6bebc18 to e70bbbb Compare September 7, 2019 09:42
@LarsDenBakker LarsDenBakker changed the base branch from master to delete-update September 7, 2019 09:49
@guybedford
Copy link
Member

@LarsDenBakker ping if you're ready for re-review.

@charlag
Copy link

charlag commented Mar 27, 2020

Hey
Is there something to be done here still?

@guybedford
Copy link
Member

@charlag I believe this work is waiting for someone to contribute the necessary work further to work through the full use case here. If you're interested in working on this please let us know.

@LarsDenBakker
Copy link
Contributor Author

Really sorry I haven't been able to spend any more time on this, it's almost there but I have some other things on my list of priorities right now :(

@charlag
Copy link

charlag commented Apr 2, 2020

No problems from my side! I would work on that but I currently don't have time/energy for that.
What could greatly help anyone interested (like me in the future) is some kind of a TODO list.

@rixo
Copy link

rixo commented Apr 2, 2020

I'm mainly taking the opportunity to thank you all for this work. Been using this in my HMR plugin for Rollup for months and never had any issue stemming from SystemJS or this PR. Thanks guys!

And I'm also curious to know what's still missing? The capacity to rerun a module without reload (because of updated dependencies) is the only thing that comes to my mind. But maybe it is a bit out of scope for the reload extra.

@guybedford
Copy link
Member

@rixo very glad to hear you've been using this successfully there. @LarsDenBakker no stress at all there are no obligations in open source (and that is also why open source is terrible :P).

The main gap in what's missing does seem to me mainly one of just getting this released and out to users - mostly even just the simple steps like docs and telling people about it. If you'd also be interested in contributing here that would be huge.

@rvion
Copy link

rvion commented Jun 25, 2020

Is there a way to help moving this forward ? :)

@guybedford
Copy link
Member

@rvion would you be interested in working on this yourself?

@guybedford
Copy link
Member

Closing due to lack of activity.

@guybedford guybedford closed this Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants