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.

Sign up for GitHub

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

Jump to bottom

[web] provide serviceWorkerVersion to the getNewServiceWorker function #130206

Closed
wants to merge 8 commits into from
Closed

[web] provide serviceWorkerVersion to the getNewServiceWorker function #130206

wants to merge 8 commits into from

Conversation

p-mazhnik
Copy link
Contributor

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

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
ditman
ditman reviewed Jul 13, 2023
View reviewed changes
Copy link
Member

@ditman ditman left a 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
@p-mazhnik
Copy link
Contributor Author

p-mazhnik commented Jul 13, 2023 •
edited

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
Copy link
Member

ditman commented Jul 13, 2023

@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
Copy link
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
Copy link
Member

@ditman ditman left a 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.

@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
@ditman
Copy link
Member

ditman commented Jul 14, 2023

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
Copy link
Member

ditman commented Jul 19, 2023

This is not normal. Pinging internally again.

@ditman
Copy link
Member

ditman commented Jul 24, 2023 •
edited

@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
Copy link
Contributor Author

p-mazhnik commented Jul 24, 2023 •
edited

@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
Copy link
Member

ditman commented Jul 24, 2023 •
edited

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
Copy link
Contributor Author

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

@ditman
Copy link
Member

ditman commented Jul 24, 2023

@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
Copy link
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