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

fix(v2): Correctly resolve sw.js path on windows #3431

Closed
wants to merge 0 commits into from
Closed

fix(v2): Correctly resolve sw.js path on windows #3431

wants to merge 0 commits into from

Conversation

ashscodes
Copy link
Contributor

Motivation

To ensure that the service worker is correctly referenced on builds on Windows operating systems. I created an issue and described the problem more here - #3420

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Tested on Windows and MacOS.

yarn start

Related PRs

N/A

@ashscodes ashscodes requested a review from yangshun as a code owner September 10, 2020 21:51
@facebook-github-bot
Copy link
Contributor

Hi @ashscodes!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Sep 10, 2020

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit d280176

https://deploy-preview-3431--docusaurus-2.netlify.app

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@slorber
Copy link
Collaborator

slorber commented Sep 11, 2020

Thanks, LGTM.

We already have a posixPath function in utils, do you think it would do the job instead? (just wondering, not very familiar to when this fn should actually be used):

image

Also noticed that our deploy previews do not have the correct manifest path due to using baseUrl, so I couldn't test the service worker on the deploy preview:

I think this is just due to our website pwa config, could you remove the leading / in pwaHead on docusaurus 2 site, so that we can test SW this more easily on the deploy preview as well?

          {
            tagName: 'link',
            rel: 'manifest',
            href: '/manifest.json',
          },

could become:

          {
            tagName: 'link',
            rel: 'manifest',
            href: 'manifest.json',
          },

And I think this should be enough to make it less sensitive to baseurl changes

@slorber slorber linked an issue Sep 11, 2020 that may be closed by this pull request
@ashscodes ashscodes requested a review from lex111 as a code owner September 11, 2020 11:39
@ashscodes
Copy link
Contributor Author

ashscodes commented Sep 11, 2020

Hi @slorber, sorry, I think I've made some mistake adding the change for the docusaurus config file on the v2 website, because I seem to have picked up someone else's commit. Apologies, first PR.

I don't think the posix path function in utils is going to strip the drive letter off the path, so the problem would persist because path.resolve() will always include it and the sw.js should always be relative to the root directory. The posix path function, at least to me, appear to be fixing directory separators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service Worker Path Is Incorrect After Yarn Build On Windows Server
4 participants