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

Register reload sources call and make 'r' restart for web #39950

Merged
merged 2 commits into from Sep 6, 2019

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Sep 6, 2019

Description

This allows 'r' to hot restart web applications, and enables the behavior in vs code and intelllij

Fixes #39848

cc @devoncarew @DanTup

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 6, 2019
@jonahwilliams jonahwilliams changed the title Register reload sources call and make 'r' restart Register reload sources call and make 'r' restart for web Sep 6, 2019
@codecov
Copy link

codecov bot commented Sep 6, 2019

Codecov Report

Merging #39950 into master will decrease coverage by 0.31%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #39950      +/-   ##
==========================================
- Coverage   57.56%   57.24%   -0.32%     
==========================================
  Files         194      194              
  Lines       18730    18728       -2     
==========================================
- Hits        10782    10721      -61     
- Misses       7948     8007      +59
Flag Coverage Δ
#flutter_tool 57.24% <50%> (-0.32%) ⬇️
Impacted Files Coverage Δ
...ools/lib/src/build_runner/resident_web_runner.dart 76.64% <50%> (+1.1%) ⬆️
.../flutter_tools/lib/src/commands/build_fuchsia.dart 13.63% <0%> (-68.19%) ⬇️
...tter_tools/lib/src/build_system/targets/macos.dart 2.38% <0%> (-45.24%) ⬇️
...ackages/flutter_tools/lib/src/resident_runner.dart 37.25% <0%> (-17.73%) ⬇️
...tools/lib/src/fuchsia/fuchsia_kernel_compiler.dart 0% <0%> (-7.7%) ⬇️
...ges/flutter_tools/lib/src/fuchsia/fuchsia_sdk.dart 64.81% <0%> (-3.71%) ⬇️
packages/flutter_tools/lib/src/run_hot.dart 65.65% <0%> (-3.24%) ⬇️
packages/flutter_tools/lib/src/version.dart 90.9% <0%> (-1.92%) ⬇️
packages/flutter_tools/lib/src/artifacts.dart 67.7% <0%> (-1.39%) ⬇️
packages/flutter_tools/lib/src/cache.dart 43.5% <0%> (-0.73%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e232560...d5076c9. Read the comment docs.

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

Fixes #39848?

@jonahwilliams jonahwilliams merged commit b4c3671 into flutter:master Sep 6, 2019
@jonahwilliams jonahwilliams deleted the reload_ier branch September 6, 2019 17:41
@DanTup
Copy link
Contributor

DanTup commented Sep 7, 2019

I have some reservations about doing this if the behaviour is the same as Hot Restart. By doing this the editor doesn't know that Hot Reload isn't available and will show the user two options (Hot Restart and Hot Reload) that behave the same, and be confusing. I think understanding the difference between Hot Reload and Hot restart is already a little confusing to new users, and having a Hot Restart that doesn't do all the things we usually say it does will only add to that.

In VS Code we have a setting to "Hot Restart on Save if Hot Reload is not available", but it's opt-in by default (because I thought it might be confusing to take you to a difference place due to not being stateful). If we want this behaviour we could change that without introducing confusion of having two buttons that do the same. Giving the editor a fake picture of capabilities makes it difficult to give the user the more appropriate UI.

That said, if the behaviour is different - for ex Devon mentioned maybe navigating back to the route from the address bar for hot reload, then I think that would be much less confusing.

@devoncarew
Copy link
Member

I think we should clearly document somewhere (cc @sfshaza2) that the behavior for the flutter web workflow for reload and restart are currently the same. Also, that reload-for-flutter-web is currently not stateful.

I believe that reload (and restart) will soon preserve the route (cc @mdebbar). That will give reload stateful-like behavior. I think it would be valuable to then have the behavior of restart changed o not preserve the route. I'm not sure exactly where the implementation change would gave to happen for that (cc @yjbanov, @jonahwilliams, @mdebbar).

@jonahwilliams
Copy link
Member Author

As far as I know, hot restart preserving the route is mostly accidental. To implement a non route preserving reload, we might register a different service extension in the web engine (reload sources?) that explicitly cleared the browser url

@devoncarew
Copy link
Member

As far as I know, hot restart preserving the route is mostly accidental.

But I do think it's useful behavior - it's a nice UX. cc @vsmenon

To implement a non route preserving reload, we might register a different service extension in the web engine (reload sources?) that explicitly cleared the browser url

I think that we look for a service extension named reloadSources as a marker for supporting hot reload, and a service extension named hotRestart as a marker for supporting hot restart. Instead of a new, web specific service extension, shouldn't thehotRestart ext. just implement the desired behavior for the web?

@jonahwilliams
Copy link
Member Author

Yes, we would probably switch the behavior around so that hotRestart was reloadSources + clear browser state

@yjbanov
Copy link
Contributor

yjbanov commented Sep 9, 2019

This doesn't have to be our permanent solution, but it's good enough for now. It is useful for iterating on a web-app despite different semantics, and it doesn't require that we change our tool protocols.

@DanTup
Copy link
Contributor

DanTup commented Sep 9, 2019

I'm not sure about IntelliJ, but VS code already had support for hot-restart-on-save, so this change doesn't enable anything new - but could add confusion to something that's already confusing (when can I reload and when do I need to restart?).

That said - in my branch I think I may have already hidden the Hot Reload button if deviceId=="chrome" (a VS Code "quirk" means we can't toggle the visibility after we launch when we can see the services). This might make it less of an issue than I was thinking, though it seems weird to be doing that given this change (though it feels equally weird to undo it).

@devoncarew
Copy link
Member

I'm not sure about IntelliJ, but VS code already had support for hot-restart-on-save, so this change doesn't enable anything new - but could add confusion to something that's already confusing (when can I reload and when do I need to restart?).

IntelliJ didn't have a setting for that - we would just not reload-on-save if the device didn't report as supporting reload.

I think the issue with a setting for restart-on-save is that we want most people (and new users especially) on the happy path workflow. If we have a setting they have to enable to get that, most users won't be aware of the setting and won't be using that workflow.

@DanTup
Copy link
Contributor

DanTup commented Sep 9, 2019

If we have a setting they have to enable to get that, most users won't be aware of the setting and won't be using that workflow.

It doesn't need to be a setting - or it could be enabled by default. I had it opt-in because I wasn't sure if auto-refreshing the page on-save would be unexpected as a default experience, but if the consensus is that that's ok, it could be the default.

That said, my concern was mostly around if it shipped to stable like this - while it's only one master I don't think it's important (users using it know it'll have quirks). If it's likely to have different behaviour (for ex. persisting the route for reload, but not for restart) before it makes a stable release, then it's likely not worth changing.

@mdebbar
Copy link
Contributor

mdebbar commented Sep 9, 2019

As far as I know, hot restart preserving the route is mostly accidental.

This is correct. It's because the url state is preserved by the browser. If we want to fully clear the url state on hot restart, that should be doable. We already do a bunch of DOM clean up on hot restart.

But as @yjbanov pointed out, this behavior is useful for iteration, especially that there's no hot reload on web.

DanTup added a commit to Dart-Code/Dart-Code that referenced this pull request Sep 10, 2019
Flutter is now registering a `reloadSources` and supports hot reload (even though behaviour is currently same as hot restart). flutter/flutter#39950
DanTup added a commit to Dart-Code/Dart-Code that referenced this pull request Oct 1, 2019
Flutter is now registering a `reloadSources` and supports hot reload (even though behaviour is currently same as hot restart). flutter/flutter#39950
DanTup added a commit to Dart-Code/Dart-Code that referenced this pull request Oct 8, 2019
Flutter is now registering a `reloadSources` and supports hot reload (even though behaviour is currently same as hot restart). flutter/flutter#39950
DanTup added a commit to Dart-Code/Dart-Code that referenced this pull request Oct 21, 2019
Flutter is now registering a `reloadSources` and supports hot reload (even though behaviour is currently same as hot restart). flutter/flutter#39950
DanTup added a commit to Dart-Code/Dart-Code that referenced this pull request Oct 21, 2019
Flutter is now registering a `reloadSources` and supports hot reload (even though behaviour is currently same as hot restart). flutter/flutter#39950
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map hot reload command to hot restart for flutter for web
7 participants