-
Notifications
You must be signed in to change notification settings - Fork 30
refactor(docx): Optimize Prop Generation Script #2786
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
base: master
Are you sure you want to change the base?
Conversation
…clude inherited React props - Introduced a new function, `filterOutInheritedReactProps`, to clean up component documentation by removing inherited props from React base attributes. - Updated the documentation processing pipeline in `generateDocs.mjs` to include this new filtering step. - Removed unnecessary `key` prop entries from various component prop JSON files to streamline documentation.
- Updated the output path for V2 props to ensure they are written in the same directory as V1 props, improving organization and accessibility of generated documentation files. - Adjusted the path construction in `generateDocs.mjs` to reflect this change.
- Simplified the `filterOutInheritedReactProps` function by removing redundant checks and improving clarity. - Moved constants outside of util functions
…ocgen-script-for-V2-components
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.
oh wow. this file is actually just empty on prod right now.
but now it has content!
Deploying atlantis with
|
| Latest commit: |
e13bce0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e58e7a2c.atlantis.pages.dev |
| Branch Preview URL: | https://rework-prop-generation-scrip.atlantis.pages.dev |
| name: Lint JavaScript with increased Node heap | ||
| command: npm run lint:js -- --quiet --format junit -o reports/junit/js-lint-results.xml | ||
| environment: | ||
| NODE_OPTIONS: "--max-old-space-size=4096" |
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.
still not 100% sure why it's only starting to fail now. while yes, there are a lot of files changed in this PR I thought this step went over everything, not just files changed meaning we'd have the same amount of files as we did before and it didn't fail...
kept hitting an out of memory error on the lint_javascript job
there are a handful of things to dig into
- typescript mismatch
- ts-eslint running on non TS files like JS/MJS
- the KeyboardAwareHOC.js from a node module seemingly being parsed
In components-native, you import react-native-keyboard-aware-scroll-view, which resolves to .../node_modules/.../KeyboardAwareHOC.js. The plugin tries to parse that file to validate import usage; with the TS parser + version mismatch, it throws and sometimes eats RAM.
so this isn't a long term fix, but it gets us past the issue and allows the rest of the benefits of these changes to come through.
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.
while yes, there are a lot of files changed in this PR I thought this step went over everything
Exactly, this does go over everything. It may have been a fluke with CI around the time you last ran this.
I'd pull in master and see if this is still necessary. FWIW I do think it's pretty common to bump node's memory limit for heavyweight jobs like linting, so I'm not against this change at all.
jdeichert
left a comment
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.
Great work!! This is lightning fast locally 👌
Something we should discuss here or soon: do we even need to commit these anymore? The only reason IIRC we committed them is because generation was so slow. Without that bottleneck, we could simply make this step part of the normal npm run dev for the docs site.
The only benefit of keeping these committed is that they act as a snapshot test... we can easily see when a PR negatively affects/breaks prop regen types. If we did start git ignoring these, we wouldn't catch little things like that and would probably want to add a bunch of tests in place.. which I don't think would catch many of the issues we see occasionally.
it would be good to read over the props and make sure all the changes are simply order, or small things like React.ReactNode -> ReactNode. I'll see if I can come up with a nicer, easier way to compare them. because while I'm 99% sure they are the same, it's very tough to tell due to the git diff reordering things, and there being so much to look over.
Is the alphabetical ordering change easy to make a separate PR for that hits master first? If we had that PR, it would likely contain the largest diff and would be easy to review, knowing only order changed. Then this PR could be updated with that... and there would probably be similar # of file changes, but the diff would be very minimal in comparison.
Leaving my approval for after this gets updated with master 👍
The alphabetical PR is optional, just would improve confidence in the diff.
| name: Lint JavaScript with increased Node heap | ||
| command: npm run lint:js -- --quiet --format junit -o reports/junit/js-lint-results.xml | ||
| environment: | ||
| NODE_OPTIONS: "--max-old-space-size=4096" |
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.
while yes, there are a lot of files changed in this PR I thought this step went over everything
Exactly, this does go over everything. It may have been a fluke with CI around the time you last ran this.
I'd pull in master and see if this is still necessary. FWIW I do think it's pretty common to bump node's memory limit for heavyweight jobs like linting, so I'm not against this change at all.
| } from "./baseComponentLists.mjs"; | ||
|
|
||
| // Load tsconfig once and build compiler options | ||
| const tsconfigPath = resolve(process.cwd(), "../../tsconfig.json"); |
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.
This is the root tsconfig.json, however components and components-native both have their own respective tsconfig.json files which take precedence. I'm not even sure if the root one is used for anything.. I guess it's a fallback for packages that don't have their own tsconfig.json.
While this works because it shares ~90% of the config, I'm curious if using the components/components-native files leads to differences in generation.
Motivations
our prop generation script takes about 60 seconds to run on my machine. it's just parsing a lot of props, and I can't help but feel like that's a long time for that sort of operation. even if it's thousands of props. they're just JSON objects really.
another small thing is that things are not in any clear order. our output is not alphabetical, and it just makes it ever so slightly harder to read. almost every other library puts props in alphabetic order.
looking at the code for the script, I noticed a lot of nested iteration. reduces inside reduces, and multiple instances of objects/documents. I wanted to take a stab at this to see if we could optimize it at all.
Changes
Added
Changed
over the last 30 days this step is around 6m, but if we look at successful runs it's closer to 4m
on this branch, 1.5m

with 1m of that in the generate + build step

vs
4m for the same step(s)

Deprecated
Removed
Fixed
Security
Testing
running
npm run generatelocally should take seconds, not minutes.the output won't be identical, it is now more deterministic with alphabetical order, and slightly different paths - though these don't really matter as long as.....
the props tables on the site all work and display our props. filtering out React/HTML props we don't want to show.
Typography now has actual props. not sure why this was getting skipped before.
it would be good to read over the props and make sure all the changes are simply order, or small things like
React.ReactNode->ReactNode. I'll see if I can come up with a nicer, easier way to compare them. because while I'm 99% sure they are the same, it's very tough to tell due to the git diff reordering things, and there being so much to look over.Changes can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.