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

feat(v2): add support to ignore files in pages plugin #3196

Merged
merged 18 commits into from
Aug 5, 2020

Conversation

anshulrgoyal
Copy link
Contributor

@anshulrgoyal anshulrgoyal commented Aug 3, 2020

Motivation

Add support to ignore files in the pages directory. It will help splitting markdown pages.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

By adding test
Working example->https://deploy-preview-3196--docusaurus-2.netlify.app/build/examples/markdownPageExample

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

docusaurus-bot commented Aug 3, 2020

Deploy preview for docusaurus-2 ready!

Built with commit c36e84d

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

this.addDependency(metadataPath);
const metadata = await readFile(metadataPath, 'utf8');
exportStr += `\nexport const metadata = ${metadata};`;
if (existsSync(metadataPath)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer async variant whenever it's not too annoying to apply like here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not totally sure of what this code do but I think we should not modify the loader there, as it's less fail-fast, and could silently ignore some important errors (like not having the createData and mdx loader paths in sync, not sure)

Instead we could modify the loader usage in pages like:

                    metadataPath: (mdxPath: string) => {
                      if (isIgnored(mdxPath)) { 
                        return undefined;
                      }
                      const aliasedSource = aliasedSitePath(mdxPath, siteDir);
                      return path.join(
                        dataDir,
                        `${docuHash(aliasedSource)}.json`,
                      );
                    },

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did same earlier but then I tried importing file in docs and blogs plugins and failed in both.

That looks good but a few things to change.

Wonder what's the behavior if I put frontmatter in a _file.md, maybe we should throw an exception explaining this is forbidden to the user. Also worth to add a test for that

Frontmatter is not in plugin control. It will require changes to mdx-loader

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will take a look on next review.

I think we could have a mdx loader option like forbidFrontmatter: (mdxPath: string) => boolean

website/docs/guides/creating-pages.md Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@slorber
Copy link
Collaborator

slorber commented Aug 3, 2020

That looks good but a few things to change.

Wonder what's the behavior if I put frontmatter in a _file.md, maybe we should throw an exception explaining this is forbidden to the user. Also worth to add a test for that

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Aug 5, 2020
@slorber slorber merged commit f234c40 into facebook:master Aug 5, 2020
@slorber
Copy link
Collaborator

slorber commented Aug 5, 2020

That's nice.

@anshulrgoyal I think we should provide this feature as a global markdown feature.

What about trying to generalize this pattern?, maybe providing in core or utils a globby wrapper that by default ignore the _ prefixed files? We'd use it in all plugins then.

Do you feel it could be useful to split docs/blogs into smaller parts as well? or maybe it's overkill for now?

@slorber slorber linked an issue Aug 7, 2020 that may be closed by this pull request
@fnune
Copy link

fnune commented Jun 15, 2021

Hey, sorry about the necro:

I've been trying to debug why my file docs/embedded/_steps/_reboot-tracking-dependency.mdx actually produces a doc page. I realized now (reading this PR) that it's because this exclusion feature is only meant to be used on pages, and not docs.

Is there an existing workaround?

@fnune
Copy link

fnune commented Jun 15, 2021

I've found a (rather nasty) workaround: put partials inside src/pages/_partials. It works because the Docusaurus Webpack setup picks up MDX there, and because the _ feature is supported only for pages, and not for docs.

It would be nice if this _ were also available for the docs directory.

@slorber
Copy link
Collaborator

slorber commented Jun 16, 2021

Yes I'd like to make _ work consistently everywhere by default

The workaround for now can be to put content in src/components-or-other-folderr/xyz.mdx as these folders are unprocessed by default (except pages). I wouldn't recommend to put it in pages as this is clearly not a page you are trying to create.

You can import it with import XYZ from "@site/src/components/xyz"

Edit: hmmm actually this probably doesn't work as there's no mdx loader for src/components being registered, so your workaround is probably the only one currently

Created another dedicated issue: #4984

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: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

src/page/_myComponent => do not create page
5 participants