Vue3: Fix OOM issue in docgen handling #28462
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Added ignore option to
vue-component-metaplugin incode/frameworks/vue3-vite/src/plugins/vue-component-meta.ts - Filters out unnecessary meta information from
node_modules - Targets object types not required by Storybook
- Aims to improve performance and prevent OOM issues
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 20fc330. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
|
We have tested this PR locally as we are facing the same OOM issue with Storybook 8+ Vue3 + Vite and can confirm this change has now resolved the issue. Thanks @et84121 for the fix! |
|
@blake-simpson Hi, what else do I need to do to get this PR merged? Do I still need to add appropriate tests? |
Hi @et84121 I'm a colleague of @blake-simpson. We just happened upon your PR as we were trying to solve the problem ourselves. We sadly don't have anything to do with @storybook/core and would also like to know what needs to be done to get this merged |
|
@blake-simpson @joeczar I apologize for the confusion. I mistakenly directed my question to the wrong person. Thank you for clarifying that. @shilman , may I ask what else I need to do to get this PR merged? Is there anything specific you'd like me to address or modify? |
shilman
changed the title
|
@et84121 Thanks so much for the fix. Really appreciate it!! I'm running CI on it now and will get a review from one of @larsrickert @chakAs3 @kasperpeulen. Once somebody gives the thumbs up, we'll merge and release it in 8.3-alpha. 8.2 should be released in the next few hours so we missed the boat on that. However if it's deemed safe, we should also be able to patch it back to 8.2.x in the next few days. Thanks for the nudge and for your patience on this! |
| (name, type, typeChecker) => { | ||
| // Omit all schemas whose declaration paths are in node_modules and are objects. | ||
| const isInNodeModules = type.getSymbol()?.getDeclarations()?.some((declaration) => { | ||
| return declaration.getSourceFile().fileName.includes('node_modules') |
There was a problem hiding this comment.
I am not sure if we can/should do it like that because that would mean the types are ignored for every node module.
So if you e.g. have a shared library where you import types from, they all will be ignored which would not be ideal and could potential break docs of existing projects that uses this.
Should we maybe only check for types of the TypeScript lib itself or let the user configure this via a framework option so its totally up to them which types they want to have excluded if they run into a performance issue?
There was a problem hiding this comment.
Even then there might be edge cases like: Pick<HTMLElement, "propA" | "propB"> which does not cause memory issues but would be ignored this way
There was a problem hiding this comment.
@larsrickert, Thank you for your thorough review and the points you've raised. I appreciate the opportunity to clarify and discuss these concerns. Let me address each point:
-
Ignoring types for all
node_modules:
I understand your concern about potentially ignoring types from shared libraries or other important modules. However, the current implementation is specifically designed to address performance issues while maintaining functionality. As mentioned in the PR description, for Storybook's specific use case, object type schemas don't significantly contribute to generating better argsTypes. -
Storybook's handling of Object type schemas:
From my understanding of the Storybook documentation (https://storybook.js.org/docs/essentials/controls#annotation), there isn't a specialized control element for Object type schemas beyond a JSON-based editor. This editor doesn't seem to benefit from having detailed object prop schemas. If my interpretation of the documentation is incorrect, please let me know. -
Edge case:
Pick<HTMLElement, "propA" | "propB">:
In this scenario, the resulting type is still an Object type. Given our current approach, ignoring the schema information for this case should not pose issues, as it aligns with our handling of other Object types. -
Handling of external type references:
For cases likeHTMLElement['id'], our implementation would still provide the corresponding string schema information. This approach ensures that we're not losing critical type information for primitive types or those referenced from external sources. -
Impact on projects using external type references:
Based on the points above, I believe the current implementation shouldn't cause problems for projects that use external type references. We're primarily optimizing for Object types while preserving other critical type information.
That being said, I'm open to discussing alternative approaches:
- We could consider limiting this optimization to TypeScript lib types only, as you suggested.
- Alternatively, we could implement this as a configurable framework option, allowing users to specify which types they want to exclude if they encounter performance issues.
I'm happy to explore these options or any other suggestions you might have to find the best balance between performance optimization and maintaining robust type support. Your insights on this matter are valuable, and I'm eager to refine this approach to best serve the Storybook community.
There was a problem hiding this comment.
@et84121 I investigated the issue a little, it seems like vue-component-meta is not the issue here since it extracts the meta successfully. But in some place we process the data further within Storybook which seems to maybe have some recursive logic which causes the out of memory error.
You can verify this by changing line 88-120 in the vue-component-meta.ts file like this:
let s = new MagicString(src);
// ...
s = new MagicString(src);
return {
code: s.toString(),
map: s.generateMap({ hires: true, source: id }),
};This will essentially clear any docgen information from further Storybook processing. The key point here is that the code runs successfully until this point so the vue-component-meta extraction works totally file.
Ideally we want to check which Storybook code causes the issue and try to fix it there instead of limiting the capabilities of vue-component-meta which could cause unintended behavior in projects :)
What do you think?
There was a problem hiding this comment.
It seems like there might be a circular reference within the args of a specific story objects. I recall encountering a similar issue before; it could be an edge case that the unit tests didn't catch.
There was a problem hiding this comment.
I think this might be related to one of my old PRs, where I addressed the unusual use of the spread operator, which could also be causing the OOM issue. It would be great to have your review and opinion, @larsrickert. Please go through my PR #22994. @et84121, your opinion is much welcome as well.
There was a problem hiding this comment.
Otherwise, with vue-component-meta, we have an option to ignore types using the schema: { ignore: ['MyIgnoredNestedProps'] }. For more details, see the README.md.
There was a problem hiding this comment.
Thank you both, @larsrickert and @chakAs3, for your thorough investigation and insights.
@larsrickert, I appreciate your deep dive into the issue. Your solution of addressing the problem within Storybook's processing, rather than limiting vue-component-meta's capabilities, is indeed a much better approach. I agree that we shouldn't restrict vue-component-meta's functionality, as it could lead to unintended consequences in other projects. I apologize for not investigating the root cause more thoroughly before proposing my solution.
@chakAs3, thank you for sharing your perspective and referencing your previous PR. While I'm not certain if the circular reference issue is directly related to the root cause here, your input is valuable in considering all potential factors.
Given these developments, I believe it would be best to close this PR and proceed with @larsrickert's solution in PR #28589. I'm glad that Storybook is addressing this issue, as it will greatly help me in advocating for Storybook's adoption as a Component Document tool in my current project.
Thank you all for your collaborative efforts in resolving this. If there's anything else I can do to help, please let me know.
|
@shilman , Thank you so much for your quick and kind response! I really appreciate your update on the process and timeline. I'm glad to hear that the PR is moving forward, and I'm happy to have contributed to the project. |
|
Based on PR review feedback, particularly the insights provided by @larsrickert (see discussion: #28462 (comment)), I believe a more optimal solution has been identified. This alternative approach addresses the issue within Storybook's processing without limiting vue-component-meta capabilities, which is preferable for maintaining functionality across various projects. Therefore, I will be closing this PR in favor of the solution proposed in PR #28589. Thank you all for your valuable input and collaboration in resolving this issue. |
|
@et84121 Thanks for contributing to Storybook and working together to fix this issue :) |
Closes #26647
Closes #26935
What I did
I fixed an Out of Memory (OOM) issue in the Vue3-Vite setup, specifically related to the vue-component-meta plugin. The root cause of the problem was that vue-component-meta was including too much unnecessary meta information when encountering native types (lib.dom.ts), which Storybook doesn't require.
To resolve this, I utilized the ignore option in vue-component-meta. The solution identifies meta information in node_modules with object types and ignores them, effectively addressing the OOM issue.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Due to some issues encountered in setting up the Storybook development environment, I've created an MVP project to demonstrate the problem and solution. To test these changes:
pnpm tsx script/gen-meta.ts HelloWorldto generate meta information forHelloWorld.vue.node_modulesare excluded.gen-meta.ts.Note: If there's a strong need for automated tests in the Storybook repository itself, please let me know, and I'll work on implementing them. Given the complexity of the Storybook setup, it might take some additional time to create a proper testing environment.
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Additional Notes
This solution assumes that for typical project requirements, external type information is not necessary. Developers can refer to the documentation of the packages they're using for relevant definitions.
For Storybook specifically, Object type schemas don't significantly contribute to generating better argsTypes. Therefore, ignoring them seems to be a suitable approach that balances functionality and performance.
While the MVP project demonstrates the fix, it's important to note that integrating this solution into the main Storybook repository may require additional considerations and testing. If the core team feels that more comprehensive testing within the Storybook environment is necessary, I'm open to working on that to ensure the solution is robust across different scenarios.