feat(theme-common, theme-classic): Improve CodeBlock Extensibility #11011
Conversation
|
Hi @Danielku15! 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 [email protected]. Thanks! |
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
⚡️ Lighthouse report for the deploy preview of this PR
|
Danielku15
force-pushed
the
feature/codeblock-extensibility
branch
from
bbfcf38 to
ee22ed9
Compare
slorber
added
the
Argos
| magicComments: [], | ||
| }); | ||
|
|
||
| const noInline = meta.options.noinline === true; |
There was a problem hiding this comment.
| const noInline = meta.options.noinline === true; | |
| const noInline = meta.options.noInline === true; |
Is there a reason to not respect the case?
There was a problem hiding this comment.
Initially I wanted to make things case insensitive so that the user-errors on using options is reduced. e.g. some having codeblocks with noInline others noinline. or ShowLineNumbers vs. showLineNumbers.
I think we have 3 options here:
- Allow case insensitive keys on the end-user side and ensure in the code we use lowercase keys. (pro: easy on end user side, con: sensitive on component side)
- Be case sensitive. (pro: easy on component side: con: risky for end user side)
- Hide the values behind a case insensitive
Map<>like abstraction. (pro: easy to use on component and end-user side, con: higher complexity in code, not easy to specify in TSX/MDX)
type CodeMetaOptions = {
get(key:string): CodeMetaOptionValue | undefined;
set(key:string, value: CodeMetaOptionValue | undefined);
delete(key:string): void;
};
For the sake of keeping this change simple I'll change things to (2).
There was a problem hiding this comment.
The former behavior was case sensitive:
const noInline = props.metastring?.includes('noInline') ?? false;A refactor shouldn't necessarily change the public API surface, and solely focus on re-organizing existing implementation.
We can consider changing that behavior, but in a dedicated PR that doesn't mix refactoring + behavior changes.
| if (showLineNumbersMeta.startsWith('showLineNumbers=')) { | ||
| const value = showLineNumbersMeta.replace('showLineNumbers=', ''); | ||
| return parseInt(value, 10); | ||
| function parseCodeBlockOptions( |
There was a problem hiding this comment.
Will need to review more in depth but that looks quite complicated
I'd prefer if we start by centralizing parsing the options, but keep the old methods like getMetaLineNumbersStart
We can eventually rewrite the parser later in another PR, but I'd prefer is we don't refactor everything at once.
Please split this into multiple sequential PRs:
- first centralize parsing
- then refactor how we parse
Not everything at once
There was a problem hiding this comment.
IMO Its fairly straight forward considering what docusaurus needs to support. If we would start from scratch, we could add some more restrictions. But this code ensures backwards compatibility with a variety of option syntaxes (no options, plain values etc.
Highlightneeds special treatment for backwards compatibilty oflineClassNames- Options without values are used for markers like
showLineNumbers - Boolean values are new, but influenced by the linked PRs in #11008 to be able to enable/disable some options.
- Number values are for
showLineNumberscompatibility.
I'll try to split apart the value handling a bit to reduce the complexity of this single function.
There was a problem hiding this comment.
Before we even think about changing the way we parse things, I'd prefer to keep the exact same parsing logic as before, extract it to a single place, and cover it widely with unit tests.
Once this is well-covered, you can start changing the implementation, ensuring all the tests keep passing.
| const title = parseCodeBlockTitle(metastring) || titleProp; | ||
|
|
||
| const {lineClassNames, code} = parseLines(children, { | ||
| const {meta, code} = parseLines(children, { |
There was a problem hiding this comment.
I don't see why parseLines should parse the metastring.
Can't we provide the meta as input?
There was a problem hiding this comment.
Can be changed. The decision behind this was:
- Keeping the existing "responsibilities" (parseLines being responsible for parsing the lines related to the codeblock).
- Assuming we will at some point in future also parse options from metacomments inside the code (see RFC), it would be
parseLinesparsing additional data intometa.
There was a problem hiding this comment.
- Keeping the existing "responsibilities" (parseLines being responsible for parsing the lines related to the codeblock).
Not sure to understand, this method's responsibility is not really parsing the metastring, but parsing lines magic comments.
- Assuming we will at some point in future also parse options from metacomments inside the code (see RFC), it would be
parseLinesparsing additional data intometa.
I'm not sure about the "Metadata Magic Comments" feature.
Even if we eventually introduce this, we'll only do this later once everything is in a better shape, and your design should be researched/challenged. It's always possible to refactor again later once we are sure we want this feature, but for now please ignore it.
| * The parsed options, key converted to lowercase. | ||
| * e.g. `"title" => "file.js", "showlinenumbers" => true` | ||
| */ | ||
| readonly options: {[key: string]: CodeMetaOptionValue}; |
There was a problem hiding this comment.
This is not ideal, not typesafe and not extendable
Let's use a CodeBlockMetaOptions interface and leverage TS declaration merging to register extra options (for example the live codeblock can register its noInline option)
There was a problem hiding this comment.
Easy to change, will do. This was initially inspired by the general code style in this area (e.g. lineClassNames). -> Also see remark regarding case insenstivity below which could influence the design of CodeBlockMetaOptions.
There was a problem hiding this comment.
This was initially inspired by the general code style in this area (e.g.
lineClassNames).
No idea what you mean 😅
Also see remark regarding case insenstivity below which could influence the design of
CodeBlockMetaOptions.
Even if we implement case insensitiveness, we still want noInline as a typesafe option attribute
| export interface Props { | ||
| readonly output: TokenOutputProps; | ||
| readonly meta?: CodeBlockMeta; | ||
| } |
There was a problem hiding this comment.
TokenOutputProps props are directly applicable to spans
I'd prefer to have
type TokenOutputProps = {
style?: StyleObj;
className: string;
children: string;
[key: string]: unknown;
};
export interface Props extends TokenOutputProps {
readonly meta?: CodeBlockMeta;
}Eventually, we could introduce a React code block context to provide the meta to all the subtree. This makes it easier to reduce coupling between code block components, by avoiding passing global context through props (less likely to break on swizzle)
There was a problem hiding this comment.
I had this before but decided not to inherit but composite because:
metashould not be applied to span. Hence you end up with something similarconst {meta, rest} = props; ... (<span {...rest} />). (I noticed that CodeBlockToken has a bug not having<span {...props.output} />like it should and<CodeBlockToken />not receivingmeta- When swizzling you can clearly differenciate between the general token inputs (like meta) and what options should be applied to the element for the output.
Generally I'm fine with the change, just wanted to be sure we consider those aspects.
There was a problem hiding this comment.
This looks awkward to me to have {...props.output}, and why we have to name a thing output in the first place. The component shouldn't be designed according to names chosen by a third-party library, it must make sense in isolation. Would you use the name "output" if you weren't using react-prism-renderer for example, and were simply designing a standalone code block token component?
If meta is accessible through context, there's no need to {meta, ...rest} because meta is not a prop.
There was a problem hiding this comment.
This PR is already quite large and hard to review
It would be better to split it into many subparts. We don't have to merge everything at once, incremental improvements are easier to merge.
@slorber
I wouldn't consider it "large", but it indeed has a bit of complexity to it. 😅 I'll split it up and refactor some bits as mentioned in the remarks.
I added some comments to give insights in my decision paths. No need to give again anwers, but it might contain some valueable bits for the split PRs.
| export interface Props { | ||
| readonly output: TokenOutputProps; | ||
| readonly meta?: CodeBlockMeta; | ||
| } |
There was a problem hiding this comment.
I had this before but decided not to inherit but composite because:
metashould not be applied to span. Hence you end up with something similarconst {meta, rest} = props; ... (<span {...rest} />). (I noticed that CodeBlockToken has a bug not having<span {...props.output} />like it should and<CodeBlockToken />not receivingmeta- When swizzling you can clearly differenciate between the general token inputs (like meta) and what options should be applied to the element for the output.
Generally I'm fine with the change, just wanted to be sure we consider those aspects.
| const title = parseCodeBlockTitle(metastring) || titleProp; | ||
|
|
||
| const {lineClassNames, code} = parseLines(children, { | ||
| const {meta, code} = parseLines(children, { |
There was a problem hiding this comment.
Can be changed. The decision behind this was:
- Keeping the existing "responsibilities" (parseLines being responsible for parsing the lines related to the codeblock).
- Assuming we will at some point in future also parse options from metacomments inside the code (see RFC), it would be
parseLinesparsing additional data intometa.
| * The parsed options, key converted to lowercase. | ||
| * e.g. `"title" => "file.js", "showlinenumbers" => true` | ||
| */ | ||
| readonly options: {[key: string]: CodeMetaOptionValue}; |
There was a problem hiding this comment.
Easy to change, will do. This was initially inspired by the general code style in this area (e.g. lineClassNames). -> Also see remark regarding case insenstivity below which could influence the design of CodeBlockMetaOptions.
| if (showLineNumbersMeta.startsWith('showLineNumbers=')) { | ||
| const value = showLineNumbersMeta.replace('showLineNumbers=', ''); | ||
| return parseInt(value, 10); | ||
| function parseCodeBlockOptions( |
There was a problem hiding this comment.
IMO Its fairly straight forward considering what docusaurus needs to support. If we would start from scratch, we could add some more restrictions. But this code ensures backwards compatibility with a variety of option syntaxes (no options, plain values etc.
Highlightneeds special treatment for backwards compatibilty oflineClassNames- Options without values are used for markers like
showLineNumbers - Boolean values are new, but influenced by the linked PRs in #11008 to be able to enable/disable some options.
- Number values are for
showLineNumberscompatibility.
I'll try to split apart the value handling a bit to reduce the complexity of this single function.
| magicComments: [], | ||
| }); | ||
|
|
||
| const noInline = meta.options.noinline === true; |
There was a problem hiding this comment.
Initially I wanted to make things case insensitive so that the user-errors on using options is reduced. e.g. some having codeblocks with noInline others noinline. or ShowLineNumbers vs. showLineNumbers.
I think we have 3 options here:
- Allow case insensitive keys on the end-user side and ensure in the code we use lowercase keys. (pro: easy on end user side, con: sensitive on component side)
- Be case sensitive. (pro: easy on component side: con: risky for end user side)
- Hide the values behind a case insensitive
Map<>like abstraction. (pro: easy to use on component and end-user side, con: higher complexity in code, not easy to specify in TSX/MDX)
type CodeMetaOptions = {
get(key:string): CodeMetaOptionValue | undefined;
set(key:string, value: CodeMetaOptionValue | undefined);
delete(key:string): void;
};
For the sake of keeping this change simple I'll change things to (2).
Even if it's not a huge PR, it still can be split into many smaller ones. I'd prefer to progress incrementally. Start by refactoring the code, without changing any behavior. Then apply fine-grained behavior changes, and justify each of them in a dedicated PR. If you refactor and change everything at once, then we may not agree on everything, and your PR is less likely to be merged fast. Let's start by focusing on what we agree on:
|
|
I fully agree on your points. Will work on adapted PRs soon-ish (just wrapping up some other general Docs parts on my project) 😉 |
Pre-flight checklist
Motivation
Closes #11008
Test Plan
Verified and tested via unit tests. I primarily focused on
codeBlockUtils.test.tsbut all other tests locally are green. Looking at the rendered codeblocks on the website they are equal to before.Test links
Deploy preview: https://deploy-preview-11011--docusaurus-2.netlify.app/
Relevant Links:
Related issues/PRs
#11008