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): use absolute path to manifest file #3554

Merged
merged 2 commits into from
Oct 8, 2020

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Oct 7, 2020

Motivation

If you visit any page other than the home page, for example https://v2.docusaurus.io/docs/, then you can see the following error in the console:

Failed to load resource: the server responded with a status of 404 () manifest.json

Or something like this:

Manifest: Line: 1, column: 1, Syntax error.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview (see motivation section).

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Oct 7, 2020
@lex111 lex111 requested a review from slorber as a code owner October 7, 2020 22:08
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 7, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 7, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 1f39ece

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

@slorber
Copy link
Collaborator

slorber commented Oct 8, 2020

Hmmm that's annoying 😅

I tried to keep relative paths because I wanted the PWA to work in deploy previews, using baseurl. Seems it does not work on the deploy preview now :'( maybe we should just prepend manually the baseurl in the config ?

@lex111
Copy link
Contributor Author

lex111 commented Oct 8, 2020

I don't get what you mean. Right now the browser cannot find the manifest file because a relative URL is specified.

@slorber
Copy link
Collaborator

slorber commented Oct 8, 2020

I mean, we want the (+) icon to be there in all cases

image

Currently, it only works on the homepage (which is the problem your PR fixes, as it will work from /docs/ too)

But your PR breaks the (+) on deploy previews: https://deploy-preview-3554--docusaurus-2.netlify.app/classic/

image

While it used to work on PRs. (but only on homepage 🤪 ): https://deploy-preview-3558--docusaurus-2.netlify.app/classic/

image


The ideal solution should make it work:

  • for all subpaths: / or /docs/myDoc
  • for deploy previews: /classic/ or /classic/docs/myDoc

This way it's easier to review PWA issues in deploy previews

@lex111
Copy link
Contributor Author

lex111 commented Oct 8, 2020

Aha, I figured it out now, so I changed to use baseUrl const, it should be OK, I guess.

@slorber
Copy link
Collaborator

slorber commented Oct 8, 2020

LGTM, we'll see if main site still works after :D

@slorber slorber merged commit 39d6787 into master Oct 8, 2020
@lex111 lex111 deleted the lex111/fix-url-to-manifest branch October 16, 2020 18:40
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: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants