Skip to content

[web] provide serviceWorkerVersion to the getNewServiceWorker function - #130206

Closed
p-mazhnik wants to merge 8 commits into
flutter:masterfrom
p-mazhnik:web-flutterjs-fix
Closed

[web] provide serviceWorkerVersion to the getNewServiceWorker function#130206
p-mazhnik wants to merge 8 commits into
flutter:masterfrom
p-mazhnik:web-flutterjs-fix

Conversation

@p-mazhnik

@p-mazhnik p-mazhnik commented Jul 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #130212

Fix Unresolved variable or type 'serviceWorkerVersion' in the _getNewServiceWorker function.

It currently works because serviceWorkerVersion exists in the index.html. But if we want to provide custom serviceWorkerVersion to the loadEntrypoint, that is not the same as generated one, _getNewServiceWorker won't work as expected.

Also, if serviceWorkerVersion will be removed from index.html, and custom version will be provided to the loadEntrypoint, we will see error in the console:

Exception while loading service worker: ReferenceError: serviceWorkerVersion is not defined 
at _getNewServiceWorker

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@github-actions github-actions Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 8, 2023
@p-mazhnik p-mazhnik mentioned this pull request Jul 13, 2023
Closed
2 tasks done
ditman
ditman reviewed Jul 13, 2023
View reviewed changes

@ditman ditman left a comment

Copy link
Copy Markdown
Member

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.

Comment thread dev/integration_tests/web/web/index_with_flutterjs_custom_sw_version.html
Comment thread dev/integration_tests/web/web/index_with_flutterjs_custom_sw_version.html Outdated
@p-mazhnik

p-mazhnik commented Jul 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

While working on this issue, I also found that we actually don't need some awaits in _getNewServiceWorker function, and I created a separate PR to address this: #130204

@ditman

ditman commented Jul 13, 2023

Copy link
Copy Markdown
Member

@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!)

@p-mazhnik

p-mazhnik commented Jul 13, 2023

Copy link
Copy Markdown
Contributor Author

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

ditman
ditman approved these changes Jul 13, 2023
View reviewed changes

@ditman ditman left a comment

Copy link
Copy Markdown
Member

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.

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 14, 2023
@p-mazhnik p-mazhnik mentioned this pull request Jul 14, 2023
Merged
8 tasks done
@ditman

ditman commented Jul 14, 2023

Copy link
Copy Markdown
Member

This one is blocked by an internal issue with the google testing check. It's being investigated.

@ditman ditman removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 14, 2023
@p-mazhnik
Merge branch 'master' into web-flutterjs-fix
64ca3cd 
# Conflicts:
#	packages/flutter_tools/lib/src/web/file_generators/js/flutter.js
@ditman

ditman commented Jul 19, 2023

Copy link
Copy Markdown
Member

This is not normal. Pinging internally again.

@ditman

ditman commented Jul 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

@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.)

@p-mazhnik

p-mazhnik commented Jul 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

@ditman

ditman commented Jul 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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!

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 24, 2023
@p-mazhnik

p-mazhnik commented Jul 24, 2023

Copy link
Copy Markdown
Contributor Author

It is pending for 2 hours already. Looks like I still need to re-create the PR?

@ditman

ditman commented Jul 24, 2023

Copy link
Copy Markdown
Member

@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!)

@ditman ditman closed this Jul 24, 2023
@p-mazhnik p-mazhnik mentioned this pull request Jul 25, 2023
Merged
@p-mazhnik

p-mazhnik commented Jul 25, 2023

Copy link
Copy Markdown
Contributor Author

@ditman new PR: #131240

auto-submit Bot pushed a commit that referenced this pull request Sep 14, 2023
@p-mazhnik
[web] provide serviceWorkerVersion to the getNewServiceWorker function (
783f2f4 
#131240)

Fixes #130212

Fix `Unresolved variable or type 'serviceWorkerVersion'` in the `_getNewServiceWorker` function.  

Supersedes #130206
@p-mazhnik
p-mazhnik deleted the web-flutterjs-fix branch September 14, 2023 22:28
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
@p-mazhnik @Mairramer
[web] provide serviceWorkerVersion to the getNewServiceWorker function (
7fa9743 
flutter#131240)

Fixes flutter#130212

Fix `Unresolved variable or type 'serviceWorkerVersion'` in the `_getNewServiceWorker` function.  

Supersedes flutter#130206
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yjbanov yjbanov yjbanov approved these changes
@ditman ditman ditman approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[web] serviceWorkerVersion is not defined at _getNewServiceWorker

3 participants

@p-mazhnik @ditman @yjbanov