fix(docusaurus-utils-validation): baseUrl + routeBasePath: allow empty string, normalized as "/" #8258
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
|
Hi @Djunnni! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, 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. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
| @@ -96,6 +96,9 @@ export const PathnameSchema = Joi.string() | |||
| '{{#label}} is not a valid pathname. Pathname should start with slash and not contain any domain or query string.', | |||
| ); | |||
|
|
|||
| // joi empty string not allowed by default. so using empty('') for custom use | |||
| export const RouteBasePathSchema = Joi.string().empty('').default(''); | |||
There was a problem hiding this comment.
Let's wait for a decision to be made in #8254 (comment)
I'd prefer to have allow('') instead of empty('') here, and not have a default value of '' => default should applied on a case by case basis.
Eventually we could use this scema for baseUrl too (rename to PathSegmentSchema?), and normalize both baseUrl + routeBasePath to have leading/trailing slashes
There was a problem hiding this comment.
When i did some of test, if use allow(''), it return ''
So i use empty('') then default('')
It gauarantee this case
joi.string().allow('').default('/') => ''
joi.string().empty('').default('').default('/') => '/'
joi.string().empty('').default('/') => '/' => this case can't gaurantee because it return undefined when i tested docusaurus-utils-validation
I found rootBasePath used '/', '', 'blogs' in this repo and then customized by default value individually
There was a problem hiding this comment.
You forgot I initially suggested to add the same code as baseurl: .custom((value: string) => addLeadingSlash(addTrailingSlash(value))),
There was a problem hiding this comment.
i also test this case you announced,
when i input empty string ""
joi.string().custom((value: string) => addLeadingSlash(addTrailingSlash(value))) => throw error
joi.string().allow('').custom((value: string) => addLeadingSlash(addTrailingSlash(value)) => return ''
# Conflicts: # packages/docusaurus-utils-validation/src/__tests__/validationSchemas.test.ts
allow '' for baseUrl + routeBasePath
|
Did some changes to the PR
I didn't want to normalize adding a trailing slash to Maybe this is possible to normalize technically, but it would require more tests to be sure. |
slorber
added
pr: bug fix
slorber
changed the title
slorber
removed
the
to backport
Pre-flight checklist
Motivation
some of packages, written about allow('') but it can be update by custom or default
so i use empty('') that is customed in joi
Test Plan
Test links
Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/
Related issues/PRs
fix #8254
#8255