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

content plugins reject options.routeBasePath = '' #8254

Closed
2 tasks done
Niko030303 opened this issue Oct 27, 2022 · 8 comments · Fixed by #8258
Closed
2 tasks done

content plugins reject options.routeBasePath = '' #8254

Niko030303 opened this issue Oct 27, 2022 · 8 comments · Fixed by #8258
Labels
bug An error in the Docusaurus core causing instability or issues with its execution good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. hacktoberfest

Comments

@Niko030303
Copy link

Have you read the Contributing Guidelines on issues?

Description

When I use default preset options

module.exports = {
  presets: [
    [
      '@docusaurus/preset-classic',
      {
        pages: {
          path: 'src/pages',
          routeBasePath: '',
          include: ['**/*.{js,jsx,ts,tsx,md,mdx}'],
          exclude: [
            '**/_*.{js,jsx,ts,tsx,md,mdx}',
            '**/_*/**',
            '**/*.test.{js,jsx,ts,tsx}',
            '**/__tests__/**',
          ],
          mdxPageComponent: '@theme/MDXPage',
          remarkPlugins: [require('remark-math')],
          rehypePlugins: [],
          beforeDefaultRemarkPlugins: [],
          beforeDefaultRehypePlugins: [],
        },
      },
    ],
  ],
};

It pointed out that ValidationError: "routeBasePath" is not allowed to be empty
image

Self-service

  • I'd be willing to address this documentation request myself.
@Niko030303 Niko030303 added documentation The issue is related to the documentation of Docusaurus status: needs triage This issue has not been triaged by maintainers labels Oct 27, 2022
@slorber
Copy link
Collaborator

slorber commented Oct 27, 2022

Hey

Somehow it's done on purpose because previously it lead to issues, although now with #3427 it shouldn't be an issue anymore.

We could either:

  • forbid '' but print a better error message suggesting to use "/" instead
  • allow '' to pass validation, as it should work fine now

Considering we have related validation for baseUrl (#8066) I guess we should do the same here, and apply this to all 3 content plugins: docs, pages, blog.

To make things more predictable I'd prefer if we apply the same logic as baseUrl and always normalize this URL segment to have leading/trailing slashes.

Cf #8066 for implementation

  baseUrl: Joi.string()
    .required()
    .custom((value: string) => addLeadingSlash(addTrailingSlash(value))),

Any opinion @Josh-Cena ?


Good issue for an external contributor looking to contribute.

Send directly the PR only if you can handle it.

Do not claim it, we won't assign it to you unless there's a PR first.

@slorber slorber added bug An error in the Docusaurus core causing instability or issues with its execution good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. documentation The issue is related to the documentation of Docusaurus status: needs triage This issue has not been triaged by maintainers hacktoberfest and removed status: needs triage This issue has not been triaged by maintainers documentation The issue is related to the documentation of Docusaurus labels Oct 27, 2022
@slorber slorber changed the title Plugin-content-pages preset options, 'routeBasePath' can't be '' content plugins reject options.routeBasePath = '' Oct 27, 2022
@slorber slorber linked a pull request Oct 27, 2022 that will close this issue
3 tasks
@Djunnni
Copy link
Contributor

Djunnni commented Oct 27, 2022

@slorber

could you check my PR? i reflect your opinion

Hey

Somehow it's done on purpose because previously it lead to issues, although now with #3427 it shouldn't be an issue anymore.

We could either:

  • forbid '' but print a better error message suggesting to use "/" instead
  • allow '' to pass validation, as it should work fine now

Considering we have related validation for baseUrl (#8066) I guess we should do the same here, and apply this to all 3 content plugins: docs, pages, blog.

To make things more predictable I'd prefer if we apply the same logic as baseUrl and always normalize this URL segment to have leading/trailing slashes.

Cf #8066 for implementation

  baseUrl: Joi.string()
    .required()
    .custom((value: string) => addLeadingSlash(addTrailingSlash(value))),

@Josh-Cena
Copy link
Collaborator

Frankly I don't get the semantic difference between "/" and "". Is the latter only useful to hack around the "multiple root plugins" issue? If that's the case I think we should continue to reject it until the actual issue is properly remedied. We already normalize slashes, but I don't think we should allow empty strings unless it has clear semantics.

@Djunnni
Copy link
Contributor

Djunnni commented Oct 27, 2022

Frankly I don't get the semantic difference between "/" and "". Is the latter only useful to hack around the "multiple root plugins" issue? If that's the case I think we should continue to reject it until the actual issue is properly remedied. We already normalize slashes, but I don't think we should allow empty strings unless it has clear semantics.

I agree with your opinion, but here, even when routeBasePath is not defined, the root (/) path is entered by default. I'm not quite sure what the difference is with "" in this case.
I think it would be better to put it as an essential element rather than to remove the ambiguous case.

@slorber
Copy link
Collaborator

slorber commented Oct 28, 2022

Frankly I don't get the semantic difference between '/' and ''. Is the latter only useful to hack around the "multiple root plugins" issue?

I don't even remember what this issue is exactly but I don't think it hacks around anything if we normalize '' to '/' anyway.

If that's the case I think we should continue to reject it until the actual issue is properly remedied.

'/' works so if we normalize '' to '/' there won't be issues, it would work as if user provided "/" 🤷‍♂️

We already normalize slashes, but I don't think we should allow empty strings unless it has clear semantics.

To me using "docs" (default plugin routeBasePath value) instead of "/docs/" is not particularly more semantically wrong than using '' instead of '/'.


routeBasePath is somehow the "plugin baseUrl". It made sense to me to be consistent between baseUrl and routeBasePath

Now I didn't notice but we actually don't accept baseUrl: '' . I thought we did normalize it to "/" already, but instead it prints an error message:

"baseUrl" is not allowed to be empty

I suggest either:

  • allow '' for both baseUrl and routeBasePath, eventually emit a warning saying the value should rather be '/'
  • reject '' for both baseUrl and routeBasePath with a clearer error message explaining to use '/' instead (+ fix docs using '')

The first option looks better to me, as users will intuitively try to use ''.

Do we want to throw on start while we could handle the bad value more gracefully?

Heck, we could even accept null/false here 🤷‍♂️

@Djunnni
Copy link
Contributor

Djunnni commented Nov 3, 2022

If the progress is stopped, how about we discuss it again?

  • allow '' for both baseUrl and routeBasePath, eventually emit a warning saying the value should rather be '/'

how about think that @Josh-Cena ?

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Nov 3, 2022

routeBasePath is somehow the "plugin baseUrl". It made sense to me to be consistent between baseUrl and routeBasePath

Yeah, I'll agree with that stance. Well, I'll prefer to normalize—I know it was previously used as a "multiple root plugins" hack, which is why I don't like ""; but if we always normalize, I do prefer the non-erroring side.

@Djunnni
Copy link
Contributor

Djunnni commented Nov 3, 2022

I know it was previously used as a "multiple root plugins" hack, which is why I don't like ""

is it happen in this repository? if some of case is exist. i want to know more. because i didn't hear vulnerabilities.

i readed docusaurus routing

routeBasePath is "plugin baseUrl".

i think it can be used "" because it works in root("/")

slorber pushed a commit that referenced this issue Dec 8, 2022
…y string, normalized as "/" (#8258)

Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com>
fix #8254
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error in the Docusaurus core causing instability or issues with its execution good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. hacktoberfest
Projects
None yet
4 participants