-
Notifications
You must be signed in to change notification settings - Fork 27.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
[web] provide serviceWorkerVersion to the getNewServiceWorker function #130206
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
Looks good to me, thanks for discovering this and preparing a fix!
A couple of minor comments. I think this should remove the global variable from the DOM so the test more closely resembles the conditions of the reported issue. Using the callback is more of a stylistic thing.
dev/integration_tests/web/web/index_with_flutterjs_custom_sw_version.html
Show resolved
Hide resolved
dev/integration_tests/web/web/index_with_flutterjs_custom_sw_version.html
Outdated
Show resolved
Hide resolved
While working on this issue, I also found that we actually don't need some awaits in |
@p-mazhnik I noticed! However you forked off of main before landing this PR, so you'll have some conflicts to resolve with yourself (hopefully easy to resolve!) |
That is ok, I just don't know which PR will land first, but I think this one will be better because it has tests |
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 LGTM. Finding a 2nd reviewer.
This one is blocked by an internal issue with the google testing check. It's being investigated. |
# Conflicts: # packages/flutter_tools/lib/src/web/file_generators/js/flutter.js
This is not normal. Pinging internally again. |
@p-mazhnik do you mind re-creating this PR in a new branch? I think it's been forgotten/ignored permanently by the tooling. (If you don't want to recreate it, I think I can do it preserving your authorship, but I'm not sure what'll happen after it merges, just let me know if you prefer me to recreate.) |
@ditman I'll re-create I saw in some PRs that merging of the latest master fixed the issue, I want to try that first, just in case |
@p-mazhnik ah yes, good idea, let's see if that gets this re-created on the google-testing tooling! Edit: It has in fact, created this issue in the FROB tool. Easy! Applying auto-submit. Thanks for your patience! |
It is pending for 2 hours already. Looks like I still need to re-create the PR? |
@p-mazhnik it's showing as "Deleted" in the internal tool as well. Yes, please re-create this from another branch so the tooling doesn't know about it! Closing this one. Apologies for the issue (and for not knowing exactly what broke!) |
flutter#131240) Fixes flutter#130212 Fix `Unresolved variable or type 'serviceWorkerVersion'` in the `_getNewServiceWorker` function. Supersedes flutter#130206
Fixes #130212
Fix
Unresolved variable or type 'serviceWorkerVersion'
in the_getNewServiceWorker
function.It currently works because
serviceWorkerVersion
exists in theindex.html
. But if we want to provide customserviceWorkerVersion
to theloadEntrypoint
, that is not the same as generated one,_getNewServiceWorker
won't work as expected.Also, if
serviceWorkerVersion
will be removed fromindex.html
, and custom version will be provided to theloadEntrypoint
, we will see error in the console:Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.