-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
aa02d1b
to
7f0cfa5
Compare
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? |
There was a problem hiding this 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 -
- 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. - 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.
Thanks for the review. And no problem, I expected that this will be some back and forth. It's quite a complex feature. |
7f0cfa5
to
4d5f52c
Compare
I've rebased on #2020 and updated the reload code. I still have some questions regarding the reload update in the other PR. |
6bebc18
to
e70bbbb
Compare
e70bbbb
to
0ce0d0a
Compare
@LarsDenBakker ping if you're ready for re-review. |
Hey |
@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. |
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 :( |
No problems from my side! I would work on that but I currently don't have time/energy for that. |
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. |
@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. |
Is there a way to help moving this forward ? :) |
@rvion would you be interested in working on this yourself? |
Closing due to lack of activity. |
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
andimport
to allow passing along theimporterSetters
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 theSystemJS
namespace instead?