-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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(core): allow plugins to declare custom route context #7082
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-7082--docusaurus-2.netlify.app/ |
Size Change: +1.85 kB (0%) Total Size: 805 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 better impl than my first attempt
// TODO this would be better to do all that in the codegen phase | ||
// TODO handle context for nested routes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these todo look still relevant
for example I don't think it works if you add a context to a doc route, only parent route context work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works everywhere, because it's handled in the same way as route components are handled. As long as there are RouteContextProvider
s, the nested routes will be properly merged. Unless "nested routes" mean something different here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think it "works" yes, but only parent routes are namespacing the context data under a "data" attribute while nested routes expose the context data
Maybe if we had a good case to test this feature on docs route we'll see if it works?
This looks fine for now, we'll fix edge cases as soon as we have a use-case for nested context
7d1c735
to
4e4a768
Compare
...genChunkNames({__context: context}, 'context', routePath, res), | ||
...(context | ||
? genChunkNames({__context: context}, 'context', routePath, res) | ||
: {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Motivation
Complete the TODO. Honestly I'm not too sure what we should put here, so I didn't modify the content plugins yet. We already have front matter duplicated twice (once in
metadata.frontMatter
, once as separatefrontMatter
injected from MDX loader), I don't really want to duplicate it a third time as that would be disastrous for bundle size.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Builds same as before
Related PRs