feat: script to generate skills from storybook#8001
Conversation
|
Hi there, I am the author of I think there are many good changes in this PR. However, I have some major concerns:
I like the changes in the folder structure + the introduction of IMO, while I have no say on this project, I think it would be beneficial to refactor the examples/props without removing the deprecation notices or documentation references (those aren't hurting anyone + I definitely see value almost every single day using them), without removing the |
Never mind if some of these points may duplicate or reinforce points already raised in the discussion) |
|
I also wonder if it is possible improve the script. For example, to split it into parser, resolver, writer, etc modules? I mean what if skill generation logic changes or grows, and it becomes hard to maintain all concerns that are mixed in a single file? And a developer should read and understand the whole script before providing new changes. Let’s say build_skills responsible for finding docs, reading mdx, parsing metadata and storybook examples, rewriting links, generating md and index, etc. Just as an example, we can have parcers folder with mdx, stories, and generators folder with component, references, etc, utils folder with some utils. |
|
Also good to think about regex parsers vs abstract syntax tree parsers for complex extraction logic to reduce fragility if docs formatting affects. |
ibutakova
left a comment
There was a problem hiding this comment.
Maybe I misunderstood or didn't get the context right, but following issues might break current system behavior (you can find more details in pr conversation):
- index.md: there seems to be a structural mismatch where index points to flat markdown paths, but the script deletes them and outputs nested directories
- wiping the entire
skills/carbon-reactdirectory before a write run looks risky - pr removes core styling and color documentation. If they are deprecated, we should document what replaces them
Also, here are a few things that would be great to have to improve long-term maintainability and AI skill quality:
- Where safe, let's have the script evaluate simple object variables and flatten them into explicit JSX props in the markdown. Or fall back to the original code + a disclaimer note for complex spreads.
- Adding high-level guidance "When to Use" (like when to choose Textbox over Textarea) could improve the skill.
- As the script handles a lot of mixed concerns right now, splitting it into separate modules (like
/parsers,/generators, and/utils) would make it much easier to maintain. Switching from regex to an AST parser for complex extractions would also make it less fragile to future documentation format changes.
Let me know what you think and if I missed any context
|
@ibutakova 2- i did not change the behavior of the script (delete all -> generate). 3- i removed colors and styles docs because they are not up to date. 4- It is something i wanted to discuss 5 - the description is meant to carry the "When to use" part. Concerning the script maintenance, i would agree but i focused on correctly generating the files. |
edleeks87
left a comment
There was a problem hiding this comment.
I've had to manually review this as the PR is too big for copilot to review so I may have missed or misunderstood something. Overall I'm happy with the approach to add more useful information to the skill files and restructure them so they're more efficient, as long as we don't lose the migration capabilities we had previously (along with anything else @quinnturner got from it etc). I do have concerns about relying on the storybook files for things like deprecations and types (less concerning as it pulls from the interfaces) as they still rely on the authors of these files not missing something at the minute. I'd feel more confident if we were still pulling things like that from parsing/inspecting the AST for a given module and then relying on the MDX etc for things like description, usage and examples etc.
| To use accordions, import the `Accordion` component and pass the desired content as child components. | ||
|
|
||
| ```javascript | ||
| import { Accordion } from "carbon-react/lib/components/accordion"; |
There was a problem hiding this comment.
comment: whilst the imports are still being included, we run the risk of this being wrong as it seems to be just reading from the MDX now which relies on them being added correctly or at all etc. Previously this was pulled by parsing and inspecting the module's AST to pull the import. In the long run we could look to have some form of linting/validation but it might be better to continue the old approach whilst we don't have that
|
|
||
| for (const mdxPath of allMdxFiles) { | ||
| const content = await fs.readFile(mdxPath, "utf8"); | ||
| const isDeprecated = content.includes("<DeprecationWarning"); |
There was a problem hiding this comment.
comment: this seems to rely on the author of whatever changes lead to a deprecation including a DeprecationWarning in the mdx. I think it would be more secure to capture this from the actual module inspecting it for a jsdoc @deprecated. We're still doing this for deprecated props but we've potentially introduced a blind spot here imo
| }); | ||
| } | ||
| storyMap.set(componentName, existing); | ||
| const props = resolvePropsForStories(storiesPath); |
There was a problem hiding this comment.
question/suggestion: it looks like we have some inconsistencies between this and reference-mdx.mjs where there's a fallback to resolveArgTypesFromMeta if resolvePropsForStories returns empty etc. I wonder if it's worth adopting the same approach here? I could very well be missing something here so apologies if this is off the mark
| ); | ||
| } else { | ||
| // Clean entire output directory then recreate structure | ||
| await fs.rm(skillsRoot, { recursive: true, force: true }); |
There was a problem hiding this comment.
suggestion: previously we were explicitly ensuring idempotency with respect to file line endings: if it was CRLF it was CRLF and if it was LF it was LF but this seems to be making it all LF. Several places even explicitly normalise input via .replace(/\r\n/g, "\n"), e.g. in mdx-discovery.mjs and reference-mdx.mjs. I wonder if it's worth adding a .gitattributes file to explicitly define how MD files in carbon handle EOL
// .gitattributes
*.md text eol=lf






What the script does :
examples/for each skill which will contain as many files as there are examples for that componentThings to decide /improve :
Problems to correct :