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

test(v2): add protocol relative uri validation test #3453

Merged
merged 3 commits into from
Sep 29, 2020

Conversation

moonrailgun
Copy link
Contributor

Motivation

I Wanna Support relative url like //docusaurus.io in footer link item in href field

Have you read the Contributing Guidelines on pull requests?

Yep

Test Plan

I append a test case for relative url in URISchema test case

or manual modify themeConfig-footer-links-items-href field start with //(not http:// or https://)

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.)

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

docusaurus-bot commented Sep 15, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 0f65fd2

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

@tommilligan
Copy link
Contributor

I've run into a similar issue already - I believe your issue will be solved by #3449 which is a more permissive implementation (will allow relative URIs such as ../<path> as well as //<path>).

If you could take a look, I'm happy to merge in the unit tests for your case.

@moonrailgun
Copy link
Contributor Author

I've run into a similar issue already - I believe your issue will be solved by #3449 which is a more permissive implementation (will allow relative URIs such as ../<path> as well as //<path>).

If you could take a look, I'm happy to merge in the unit tests for your case.

@tommilligan
Well, I check your PR. I also use joi's allowRelative at first. But i think relative uri like a simple string is not a good URI schema check. Then i write it manual.

@moonrailgun
Copy link
Contributor Author

For your case, why not use to field? I have no idea about the reason of need full reload

@tommilligan
Copy link
Contributor

The reason for a full reload is I'm linking to an external, but relative path. In my case I'm linking from say, example.com/docs which is the root of our docusaurus app, to example.com, which is an independent static site. They do not share JS code, are not the same SPA etc.

A full reload is required, otherwise docusaurus simply interprets the relative link as requiring a history.push(...) call, which triggers an internal JS component load but not a full HTTP page reload. This leads to a 404 page.

To force the full reload, I set href and target, as this cannot be achieved with to as far as I'm aware.

@moonrailgun
Copy link
Contributor Author

@tommilligan

Its looks like swizzle can be helpful for your case.

try type in your cli

npx docusaurus swizzle @docusaurus/theme-classic Navbar

and modify your own navbar if your pull request not been approval. In my mind its should not a common way.

for offical document: https://v2.docusaurus.io/docs/cli/#docusaurus-swizzle

@tommilligan
Copy link
Contributor

@moonrailgun actually, I've just tried setting both to and target as you suggested and it works - thanks! I guess in that case I'm confused as to why both to and href are allowed, but 🤷

Thanks for your help!

@lex111 lex111 requested a review from slorber September 16, 2020 07:07
# Conflicts:
#	packages/docusaurus-utils-validation/src/__tests__/validationSchemas.test.ts
@slorber
Copy link
Collaborator

slorber commented Sep 29, 2020

Thanks,

Actually updated the PR but kept the test cases, because @tommilligan is right, // is included in the {relativeUri: true} option (that I merged).

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2020

BTW, it's worth nothing that relative protocol uris are considered an antipattern according to https://www.paulirish.com/2010/the-protocol-relative-url/
In 2020 this should be fine to always hardcode to https

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2020

@moonrailgun actually, I've just tried setting both to and target as you suggested and it works - thanks! I guess in that case I'm confused as to why both to and href are allowed, but 🤷

This is a confusing part of this project, and is historically like this. Will try to improve that but it's not so easy to make changes without annoying users with breaking changes unfortunately

@slorber slorber changed the title feat(v2): add relative uri support test(v2): add protocol relative uri validation test Sep 29, 2020
@slorber slorber added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Sep 29, 2020
@slorber slorber merged commit f802a76 into facebook:master Sep 29, 2020
@lex111 lex111 added this to the v2.0.0-alpha.65 milestone Sep 29, 2020
@moonrailgun
Copy link
Contributor Author

@slorber Well, i just consider about should simple text should identified as a uri?

like relativeURL.

Maybe i am too prudent.

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.

6 participants