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 #3436

Merged
merged 2 commits into from
Sep 11, 2020
Merged

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

merged 2 commits into from
Sep 11, 2020

Conversation

ashscodes
Copy link
Contributor

Apologies, messed up here (#3431 - first attempt at a PR on GitHub) and thought it was best to re-submit.

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

Added change to docusaurus.config.js as per @slorber's comment here

I don't think the posix path function in utils is going to strip the drive letter off the path if it was used, 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.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Tested on Windows and MacOS.

yarn start

Related PRs

N/A

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 11, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Sep 11, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 563e2eb

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

@slorber
Copy link
Collaborator

slorber commented Sep 11, 2020

thanks @ashscodes , LGTM, will merge it, just fixed the icons to see if this solves my inability to test the deploy previews PWA install mode (think icon is required to make it work)

@ashscodes
Copy link
Contributor Author

Great. Sorry, I forgot it would probably need the preceding / off the images too.

@slorber
Copy link
Collaborator

slorber commented Sep 11, 2020

No problem :) thanks

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Sep 11, 2020
@slorber slorber merged commit 37989a0 into facebook:master Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants