Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add API to Renderer to trigger a UI refresh on hot reload #30884
Add API to Renderer to trigger a UI refresh on hot reload #30884
Changes from 8 commits
55e88af
7311d25
eec977a
89b9b4b
ee77867
cebf3bf
b9e9b5a
faadec8
aea65f3
5bbe396
d9e3b4d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there any chance that a static constructor prevents the linker from removing the type? I would guess that static constructors may always have side effects and hence always have to be left in and will run.
It's probably not a big deal. I'll leave it up to you whether you think it's worth doing in a less convenient way!
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.
Actually I have no idea how smart the linker is about figuring out whether the static constructor would run or not.
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.
Static ctors are hard to trim away and guarantee correctness. I believe the only time a static ctor will be trimmed is if the whole type can be trimmed. Maybe also if it was
BeforeFieldInit
and all the static fields were removed?@vitek-karas and @MichalStrehovsky would know the exact rules here.
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 think the rule is basically:
BeforeFieldInit
then linker may trim it.cctor
is preserved only if there's a field preserved on the typeThere 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 was able to do some "fun" substitutions.xml to trim references to this type. Here's what the trimmed results look like now:
Removing the interface and the context is difficult because
ComponentBase
implements it. If we were determined we could turn the context in to aFunc<bool>
that would eliminate the context type.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.
Do we need/want a
feature
value here? Or is the idea that whenever you are trimming this assembly, turn off HotReload?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.
The latter. We don't have any scenarios where we would want hot reload to be present in a published application, so it just seems prudent to trim it all the time. Do you foresee any reason to put this behind a switch?
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.
No, not at this time. I was just making sure my assumption was correct. Maybe sticking a quick comment in the xml file would help.
Just an FYI - in
dotnet/runtime
we also run the trimmer on each shared fx assembly during the build in order to trim away any dead code (which can be there because of shared source files), and in System.Private.CoreLib it will trim the IL for specific OS and architecture, since CoreLib targets each separately. If you were to ever run the trimmer in this way, this file would still take affect and it would trim away the HotReload stuff during your build. I don't think you have plans to do this, but I just wanted to call it out for informational purposes.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.
We do want these types to be present when users are developing, so we couldn't trim it as part of our build. Our codebase has fewer platform specific implementations (DataProtection is the only one that comes to mind which has lots of windows specific code).
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.
FYI @davidfowl
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.
Can we replace this with a property on
RenderHandle
? e.g.RenderHandle.IsHotReloading
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.
This also brings up the point that, in some applications, hot reload refreshes might not be as state-preserving as you'd want. Consider if a root (or deep) component always does some async data loading while showing
Loading...
instead of its child content, then instantiates its descendant subtree. In that case, each code delta is going to retrigger this flow, and you won't get the instant update in the UI you're actually working on (you'll see "Loading..."), and any transient state in descendants will be discarded.I'm not disputing that you're doing the right thing by re-rendering the whole tree (I'm disputing the async thing, but that's separate). I'm just realising that while our demo cases hot-reloaded really well, there will be some other cases that don't. It will be up to app developers to make their component hierarchies refresh in a nondestructive way to get the best experience. For example they should try to do data loading in
OnInitializedAsync
rather thanOnParametersSetAsync
(except if they manually compare old and new parameters) so that each hot reload doesn't rebuild the descendant tree from scratch. We should think about how to set this expectation in how we document and communicate the feature (cc @danroth27).