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

Jump to bottom

Removed service worker #12974

Merged
merged 13 commits into from Mar 16, 2021
Merged

Removed service worker #12974

merged 13 commits into from Mar 16, 2021

Conversation

nickytonline
Copy link
Contributor

@nickytonline nickytonline commented Mar 11, 2021 •
edited by mstruve

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

This removes all of our service worker functionality with a self-destructing service worker which appears to be the best approach for unregistering a service worker. See https://github.com/NekR/self-destroying-sw#how-to-use.

Related Tickets & Documents

#12899
#12631
#12000
#11606
#11533
#11488
#11270
#10293
#10173
#9657
#9642
#9509
#9254
#9228
#8729
#7339
#7081
#6497

QA Instructions, Screenshots, Recordings

Test this in Chrome, Firefox, and Safari. Note in Safari, there may be no service worker registered due to the work done with the service-companion.js file. In that case, ensure that no errors are thrown in the console of the web dev tools in Safari.

  1. Pull down latest of the main branch and deploy the site locally. Navigate to the home page and validate that the service worker is running by opening the developer tools and navigating to the developer tools.

Chrome

image

Firefox

image

Safari

image

  1. Pull down the branch for this PR and deploy it locally. Refresh the browser window that is currently open and validate that the service worker gets unregistered.

Chrome

image

Firefox

image

Safari

image

UI accessibility concerns?

N/A this is front-end infrastructure

Added tests?

  • Yes
  • No, and this is why: I've removed tests related to the service worker, so existing tests should continue to pass.
  • I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

  • I've updated the Developer Docs and/or
    Admin Guide, or
    Storybook (for Crayons components)
  • I've updated the README or added inline documentation
  • @mstruve will share this change in a Changelog
    or in a forem.dev post
  • I will share this change internally with the appropriate teams
  • I'm not sure how best to communicate this change and need help
  • This change does not need to be communicated, and this is why not: please
    replace this line with details on why this change doesn't need to be
    shared

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

Samuel L. Jackson in Jurassic Park saying "Hold on to your butts"

@nickytonline nickytonline added area: service worker tech: fullstack changes will heavily involve both frontend and backend technologies labels Mar 11, 2021
@pr-triage pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Mar 11, 2021
nickytonline
nickytonline commented Mar 11, 2021
View reviewed changes
app/assets/javascripts/base.js.erb Show resolved Hide resolved
app/controllers/application_controller.rb Show resolved Hide resolved
app/lib/url.rb Show resolved Hide resolved
app/views/layouts/application.html.erb Show resolved Hide resolved
app/views/service_worker/index.js.erb Show resolved Hide resolved
@nickytonline nickytonline marked this pull request as ready for review March 11, 2021 22:29
@nickytonline nickytonline requested a review from a team March 11, 2021 22:29
@nickytonline nickytonline requested a review from a team as a code owner March 11, 2021 22:29
@nickytonline nickytonline requested review from ludwiczakpawel and removed request for a team March 11, 2021 22:29
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Mar 11, 2021
@nickytonline
Copy link
Contributor Author

I've got some rspec test failures. I can look at those later tonight unless someone wants to jump on that.

fdocr
fdocr reviewed Mar 11, 2021
View reviewed changes
Copy link
Contributor

@fdocr fdocr left a comment

Choose a reason for hiding this comment

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

Haven't had a chance to test it yet but left an initial question inline

app/views/service_worker/index.js.erb Show resolved Hide resolved
citizen428
citizen428 reviewed Mar 12, 2021
View reviewed changes
app/lib/url.rb Outdated Show resolved Hide resolved
@nickytonline nickytonline requested review from Zhao-Andy and removed request for ludwiczakpawel and jacobherrington March 12, 2021 15:06
joshpuetz
joshpuetz approved these changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@joshpuetz joshpuetz left a comment

Choose a reason for hiding this comment

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

Tested locally in chrome and successfully removed an already installed service worker. 👍🏻

app/controllers/service_worker_controller.rb Show resolved Hide resolved
@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Mar 12, 2021
@pr-triage pr-triage bot added the PR: partially-approved bot applied label for PR's where a single reviewer approves changes label Mar 12, 2021
@liflovs
Copy link

liflovs commented Mar 13, 2021 •
edited

@nickytonline does this commit help with seo problem in #9509?

@nickytonline
Copy link
Contributor Author

nickytonline commented Mar 13, 2021 •
edited

@nickytonline does this commit help with seo problem in #9509?

@liflovs , it should but I think there may be some additional dead code to remove based on @benhalpern's comment #9509 (comment). Going to out this back into draft mode to validate this next week.

@nickytonline nickytonline marked this pull request as draft March 13, 2021 13:28
@citizen428
Copy link
Contributor

Tested with Chromium Edge on Windows, works as expected here too 👍

@pr-triage pr-triage bot added PR: draft bot applied label for PR's that are a work in progress and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Mar 15, 2021
@rhymes rhymes requested a review from aitchiss March 16, 2021 11:28
@Zhao-Andy
Copy link
Contributor

Just a heads up that I restarted the failed build Travis but maybe I didn't need to 😅

mstruve
mstruve approved these changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

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

🚀

@nickytonline nickytonline marked this pull request as ready for review March 16, 2021 19:55
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Mar 16, 2021
@nickytonline nickytonline merged commit 4fb6230 into master Mar 16, 2021
@nickytonline nickytonline deleted the nickytonline/remove-service-worker branch March 16, 2021 19:55
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: unreviewed bot applied label for PR's with no review labels Mar 16, 2021
mstruve added a commit that referenced this pull request Mar 16, 2021
@mstruve
Revert "Removed service worker (#12974)"
5e02ae0 
This reverts commit 4fb6230.
@mstruve mstruve mentioned this pull request Mar 16, 2021
Merged
mstruve added a commit that referenced this pull request Mar 16, 2021
@mstruve
Revert "Removed service worker (#12974)" (#13014)
2416387 
This reverts commit 4fb6230.
nickytonline added a commit that referenced this pull request Mar 16, 2021
@nickytonline
Revert "Revert "Removed service worker (#12974)" (#13014)"
438699a 
This reverts commit 2416387.
@nickytonline nickytonline mentioned this pull request Mar 16, 2021
Merged
14 tasks
nickytonline added a commit that referenced this pull request Mar 17, 2021
@nickytonline
Revert "Revert "Removed service worker (#12974)" (#13014)" (#13015)
7c1343b 
This reverts commit 2416387.
@nickytonline nickytonline added this to DONE 🎉 in Current Cycle Work Mar 21, 2021
@cmgorton cmgorton mentioned this pull request Mar 23, 2021
Closed
@citizen428 citizen428 mentioned this pull request Aug 30, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshpuetz joshpuetz joshpuetz approved these changes

@citizen428 citizen428 citizen428 approved these changes

@mstruve mstruve mstruve approved these changes

@fdocr fdocr fdocr approved these changes

@maestromac maestromac maestromac approved these changes

@benhalpern benhalpern Awaiting requested review from benhalpern

@Zhao-Andy Zhao-Andy Awaiting requested review from Zhao-Andy

@aitchiss aitchiss Awaiting requested review from aitchiss

Assignees
No one assigned
Labels
merge by any core This tag is a signal that you are okay with any core team member merging your code for you. PR: merged bot applied label for PR's that are merged tech: fullstack changes will heavily involve both frontend and backend technologies
Projects
No open projects
Current Cycle Work
DONE 🎉
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@nickytonline @jgaskins @fdocr @liflovs @citizen428 @Zhao-Andy @joshpuetz @mstruve @maestromac