-
Notifications
You must be signed in to change notification settings - Fork 65
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
Audit unused packages #595
Comments
We can start with this:
(from https://gist.github.com/BSFishy/08d48577849391eb08c9313ae632574b) |
I am not aware of any tools that perform this check. You can see in #595 (comment) I attempted to make a little script to perform this, but that script most definitely has some issues and may be better used as a starting point rather than a de-facto source of truth.
Issue #594 is still in progress, and may be for a good amount of time. We're working on it incrementally, because a full upgrade of every package in the list of dependencies would involve a lot of moving parts (just take a look at #952, #949, #770, etc), so it's easier to tackle for all parties involved if it's taken incrementally. I think this can be done incrementally in tandem. We can do an initial pass now, and as we improve our dependencies, we can continue work on working out resolutions and unused packages. |
@BSFishy, Got it. Let's start the review incrementally. Although it is going to take time, I think. First step, I will check all the listed dependencies you mention in comment #595 (comment) and check if they are being used or not. What do you think? |
Yeah, I think that's a great starting place |
Just finished an entire review ONLY for the dependencies (not devDependencies). Unfortunetely, the list you gave before is not entirely accurate, so what I did was to remove one by one each dependency with a test after the removal. Below a lists of the dependencies that upon removal, the tests still pass and the oui website documentation load with no apparent problem. "chroma-js": "^2.4.2",
"lodash": "^4.17.21",
"@types/resize-observer-browser": "^0.1.7",
"vfile": "^4.2.0",
"mdast-util-to-hast": "^10.0.0",
"prop-types": "^15.8.1",
"react-is": "~16.3.0",
"rehype-raw": "^5.0.0",
"rehype-stringify": "^8.0.0",
"unist-util-visit": "^2.0.3",
"url-parse": "^1.5.10",
"uuid": "^9.0.0" Here the entire command to remove all these packages if you want to test on your side yarn remove chroma-js lodash @types/resize-observer-browser vfile mdast-util-to-hast prop-types react-is rehype-raw rehype-stringify unist-util-visit url-parse uuid Two questions regarding these results:
Regards, Samuel |
Thanks for doing this so far. I've written a new (smarter but still dumb) script script, which reads whatever is currently in
HOWEVER, I know some of those dependencies are used elsewhere, outside of those directories. For example, we use The reason I did this is because I can see some of the dependencies you listed are most definitely used. For example, oui/src/components/badge/badge.tsx Line 40 in a90eadd
I think ultimately, the best way to figure out if a dependency is used is to manually validate. Meaning to do a full project search for the dependency name and manually validating false positives and negatives.
I'll also say that understanding each individual dependency and how it may interact with our setup helps a lot. For example, a lot of tools don't come with a CLI built in, so you need to install a CLI package for that. E.g. Hope this helps |
As part of this, I'd also like to see dependencies get put in their correct sections. For example, it doesn't make much sense to me that |
Hi, @BSFishy . @BigSamu and I talked yesterday about this issue, and we were wondering if I could be added to the issue so that he and I can work on it together? What would be the best way for us to work on the same branch, since we have our own separate forked repos? My thought is that he could give me contributor access to the branch he is doing the audit on. However, I wanted to run that idea by you and see if there is a better way to do things. |
Yeah, that sounds great. It really depends on how close you're working on things. If the surface area you're covering is relatively small, contributor permissions would probably be the way to go. The other methods that come to mind are making another fork off of @BigSamu's fork and making PRs against that one, then make one final PR to this repo, or making a feature branch in this repo then both of you can make PRs into that branch, but that's usually reserved for larger, long-running projects. Given this will most likely mainly be changes to |
Hi @BSFishy, yeah I think the contributor approach will be the way to go. The review of the packages is big for this issue. So some extra hands would be really appreciated. Just wonder if when we finished this review and send a PRs, this PRs will be co-author by both of us, correct? |
Only one person will make the PR, however the commits within the PR will have their respective authors. You can look into how to add multiple authors to a commit here: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors (make sure you're also signing the commits off too!) From there, when the PR is merged, it will show up as being authored by both of you |
Great! Thanks! We will take a look to that! |
I am starting the review of the Regards, Samuel |
Continue with the reviewing of the runtime dependencies and taking into consideration your comment above the list below enumerate the packages that are not being used "@types/resize-observer-browser": "^0.1.7",
"rehype-raw": "^5.0.0",
"rehype-stringify": "^8.0.0" Also, in the same original the following packages should change their classification from "@types/chroma-js": "^2.4.0",
"@types/lodash": "4.14.192",
"@types/numeral": "^2.0.2",
"@types/react-beautiful-dnd": "^13.1.3",
"@types/react-input-autosize": "^2.2.1",
"@types/react-virtualized-auto-sizer": "^1.0.1",
"@types/react-window": "^1.8.5",
"@types/refractor": "^3.0.0",
"@types/resize-observer-browser": "^0.1.7", Here the proper commands for this updates: yarn remove @types/resize-observer-browser rehype-raw rehype-stringify
yarn remove @types/chroma-js @types/lodash @types/numeral @types/react-beautiful-dnd @types/react-input-autosize @types/react-virtualized-auto-sizer @types/react-window @types/refractor
yarn add --dev @types/chroma-js @types/lodash @types/numeral @types/react-beautiful-dnd @types/react-input-autosize @types/react-virtualized-auto-sizer @types/react-window @types/refractor Sending this message to update @JohnathonBowers. @JohnathonBowers I will start the review with this list shared by @BSFishy from top to bottom. Then later, we can check the entire list
|
@BigSamu That sounds good. I can start from the bottom of the list and work up. |
As @BSFishy mentioned in his post above, remember to use the following scripts for checking the list of devDependencies he shared with us yarn start
yarn build
yarn build-docs
yarn test
yarn lint |
Ok, some progress has been made in this issue. Here a google docs we are using for the review of the dev dependencies according to the list given by @BSFishy above. Review for runtime dependencies already already finished. |
@BSFishy I'm working up from the bottom of the list you gave to @BigSamu . The first dependency I found that was unused in the source code is |
Ok @Willie-The-Lord is joining efforts here to continue working on this issue. From my side, I finished a review in the dev dependencies from package NOTE: One milestone has been reached with the review of the runtime dependencies and reclassification of dev dependencies wrong classified under runtime dependencies. Please check this last commit in the working repo. @BSFishy two questions for you:
Regards, Samuel |
Yeah, checking the
Let's create a new issue for this and discuss it there.
This tool looks like a smarter version of the script I wrote. I think ideally, a tool will actually parse and understand how code is included in the project, which it looks like depcheck does. Of course, a final manual review just to be 100% sure it's accurate in the context of this project would be ideal, since every project is different and for that reason, these tools can't be 100% accurate 100% of the time. But yeah, for sure see what that tool says about OUI! |
@BSFishy, Issue #1130 created for this.
Ok I will use it for a final check! |
@Willie-The-Lord I've checked the dependencies from the bottom of Matt's list up until the |
Hi @JohnathonBowers I will be checking the dependencies later today! |
@BigSamu Just audited the unused packages and push the changes to your branch. I also updated the google doc. Thanks! |
Many thanks @Willie-The-Lord! @BSFishy, a resume so far of the work
"@types/resize-observer-browser": "^0.1.7",
"rehype-raw": "^5.0.0",
"rehype-stringify": "^8.0.0"
"moment-timezone": "^0.5.41",
"eslint-plugin-jest": "^24.1.0",
"@babel/plugin-proposal-class-properties": "^7.10.4",
"@babel/plugin-proposal-object-rest-spread": "^7.11.0",
"@types/chroma-js": "^2.4.0",
"@types/lodash": "4.14.192",
"@types/numeral": "^2.0.2",
"@types/react-beautiful-dnd": "^13.1.3",
"@types/react-input-autosize": "^2.2.1",
"@types/react-virtualized-auto-sizer": "^1.0.1",
"@types/react-window": "^1.8.5",
"@types/refractor": "^3.0.0",
➜ oui git:(clean/unused-packages) ✗ depcheck [🐍 system]
Unused devDependencies
* @babel/cli
* @babel/plugin-transform-async-to-generator
* @babel/plugin-transform-runtime
* @svgr/plugin-jsx
* @svgr/plugin-svgo
* @types/jest
* babel-core
* babel-jest
* cache-loader
* core-js
* enzyme-to-json
* eslint-import-resolver-webpack
* postcss-cli
* pre-commit
* webpack-cli
Missing dependencies
* history: ./src-docs/src/routes.js
* variables-from-scss: ./src-docs/src/views/utility_classes/utility_classes_responsive.js
* prop-loader: ./src-docs/src/views/tables/sorting/sorting_section.js
* @babel/types: ./src-docs/src/views/panel/playground.js
* brace: ./src-docs/src/views/code_editor/code_editor.js
* @babel/template: ./src-docs/src/services/playground/utils.js
* @opensearch-project/oui: ./src/components/index.d.ts
* pegjs-inline-precompile: ./src/components/search_bar/query/default_syntax.ts
* portal: ./src/components/portal/__snapshots__/_index.scss
* vfile-message: ./src/components/markdown_editor/markdown_editor.tsx
* unist: ./src/components/markdown_editor/markdown_types.ts
* cheerio: ./src/components/icon/icon.test.tsx
* postcss: ./scripts/compile-themes.js
* <%= fileName %>: ./generator-oui/component/templates/_index.scss
Regards, Samuel |
Furthermore, I just found this package: https://www.npmjs.com/package/@vercel/nft, which is meant to identify files needed at runtime. I wrote a quick script to run this on OUI: https://gist.github.com/BSFishy/f917770acac228565e98d9d819d582e5 and got this output:
Again, this isn't a catch-all and should be taken with a grain of salt, but this could be a good starting place for our |
Thanks @BSFishy. Let's organize ourselves to take a look to the runtime dependencies again. @Willie-The-Lord, @JohnathonBowers, Now I am having issues to run our branch, not sure why? I was working in other issues in other branches and when I got to this branch I did a start refresh as always running the following: rm -rf node_modules yarn.lock
yarn clean
yarn install Then I ran ...<More Lines to the Top>
137 expect(instance.isNodeOpen(items[1])).toBe(true);
~~~~~~~~~~
src/components/tree_view/tree_view.test.tsx:139:21 - error TS2339: Property 'isNodeOpen' does not exist on type 'Component<{}, {}, any>'.
139 expect(instance.isNodeOpen(items[0])).toBe(true);
~~~~~~~~~~
src/components/tree_view/tree_view.test.tsx:141:14 - error TS2339: Property 'handleNodeClick' does not exist on type 'Component<{}, {}, any>'.
141 instance.handleNodeClick(items[0]);
~~~~~~~~~~~~~~~
src/components/tree_view/tree_view.test.tsx:142:21 - error TS2339: Property 'isNodeOpen' does not exist on type 'Component<{}, {}, any>'.
142 expect(instance.isNodeOpen(items[0])).toBe(false);
~~~~~~~~~~
src/utils/deprecated/deprecated.tsx:36:15 - error TS2786: 'Component' cannot be used as a JSX component.
Its element type 'ReactElement<any, any> | Component<any, any, any> | null' is not a valid JSX element.
Type 'Component<any, any, any>' is not assignable to type 'Element | ElementClass | null'.
Type 'Component<any, any, any>' is not assignable to type 'ElementClass'.
The types returned by 'render()' are incompatible between these types.
Type 'React.ReactNode' is not assignable to type 'import("/Users/samuel/Project_Code/OpenSearch_Initiative_Programme/oui/node_modules/@types/enzyme/node_modules/@types/react/ts5.0/index").ReactNode'.
36 return <Component {...props} />;
~~~~~~~~~
Found 305 errors in 43 files.
Errors Files
176 node_modules/@types/enzyme/node_modules/@types/react/ts5.0/index.d.ts:3290
6 node_modules/@types/react/index.d.ts:3127
1 src-docs/src/components/guide_section/guide_section.tsx:186
3 src-docs/src/components/guide_section/guide_section_parts/guide_section_tabs.tsx:99
2 src-docs/src/views/page/_page_demo.tsx:123
12 src/components/accessibility/keyboard_accessible.test.tsx:73
1 src/components/accordion/accordion.test.tsx:219
1 src/components/accordion/accordion.tsx:316
2 src/components/basic_table/basic_table.tsx:749
1 src/components/bottom_bar/bottom_bar.tsx:233
5 src/components/combo_box/combo_box.test.tsx:444
1 src/components/combo_box/combo_box.tsx:1010
1 src/components/context_menu/context_menu_panel.tsx:539
1 src/components/control_bar/control_bar.tsx:475
1 src/components/copy/copy.test.tsx:63
1 src/components/datagrid/data_grid_body.tsx:691
1 src/components/datagrid/data_grid_cell.tsx:171
1 src/components/datagrid/data_grid_cell_buttons.tsx:95
3 src/components/datagrid/data_grid_cell_popover.tsx:94
1 src/components/datagrid/data_grid_control_header_cell.tsx:201
2 src/components/datagrid/data_grid_inmemory_renderer.tsx:98
2 src/components/date_picker/date_picker.tsx:241
16 src/components/date_picker/super_date_picker/super_date_picker.test.tsx:55
5 src/components/delay_hide/delay_hide.test.tsx:39
2 src/components/error_boundary/error_boundary.test.tsx:50
1 src/components/flex/flex_grid.tsx:126
1 src/components/flex/flex_item.tsx:83
2 src/components/flyout/flyout.tsx:394
1 src/components/focus_trap/focus_trap.test.tsx:219
1 src/components/icon/icon.tsx:811
1 src/components/markdown_editor/markdown_editor.tsx:473
1 src/components/observer/mutation_observer/mutation_observer.test.tsx:48
1 src/components/observer/resize_observer/resize_observer.test.tsx:73
4 src/components/outside_click_detector/outside_click_detector.test.tsx:43
1 src/components/popover/input_popover.tsx:133
3 src/components/popover/popover.tsx:786
1 src/components/popover/wrapping_popover.tsx:74
1 src/components/portal/portal.test.tsx:39
26 src/components/resizable_container/resizable_container.test.tsx:43
1 src/components/text_diff/text_diff.tsx:97
2 src/components/tool_tip/tool_tip.tsx:333
8 src/components/tree_view/tree_view.test.tsx:112
1 src/utils/deprecated/deprecated.tsx:36
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. Any suggestions what could be? Regards, Samuel |
Ok, I found the problem. It happens when you remove |
Now would be a good time to go through the list of packages in OUI and the list of resolutions and figure out if they're still necessary. It's possible that over time, packages have stopped being used or resolutions are no longer necessary.
It might be better to take care of this task after we complete #594
The text was updated successfully, but these errors were encountered: