[OSCI][CLEAN] Audit unused dependencies in OUI #1135

BigSamu · 2023-11-01T16:01:42Z

Description

Research was done to clean dependencies not used in OUI. Review finished with actions detailed below, but further work has to be done after resolving issue #594

Actions Performed

Following runtime dependencies or devDependencies were removed:

"@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",

Following dependencies below were found deprecated and issue [OSCI] Plugin Babel Dependencies Deprecated (Proposal-Class-Properties and Proposal-Object-Rest-Spread) #1130 was created to address this.

"@babel/plugin-proposal-class-properties": "^7.10.4",
"@babel/plugin-proposal-object-rest-spread": "^7.11.0",

Following dependencies below, were reclassified as devDependencies instead of runtime dependencies

"@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",

Issues Resolved

Issue #595

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

Signed-off-by: Johnathon Bowers <[email protected]>

Signed-off-by: Willie Hung <[email protected]>

…into clean/unused-packages

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

…into clean/unused-packages

BSFishy

I want to hold off on approving this until we have integration testing with OSD, just to be as safe as possible

CHANGELOG.md

JohnathonBowers

I checked out this PR on my local machine. I was able to get the OUI site to run without issue. Also, I ran yarn build, yarn build-docs, and yarn test, and everything ran successfully. The suggested changes look good to me, although I know that, as @BSFishy said, we still need to wait for the integration tests to be functional.

Co-authored-by: Matt Provost <[email protected]> Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BigSamu · 2023-11-04T13:57:06Z

I want to hold off on approving this until we have integration testing with OSD, just to be as safe as possible

Yeah, sure @BSFishy. Make sense!

willie-hung · 2023-11-06T05:52:05Z

I want to hold off on approving this until we have integration testing with OSD, just to be as safe as possible

Yeah I agree!

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

…into clean/unused-packages

BigSamu · 2024-01-01T16:05:52Z

Hi @BSFishy, @joshuarrrr

Can we retake this Pull Request? What else is required for the merge? I recently updated it to align with the main branch.

BSFishy · 2024-01-02T18:31:42Z

Can we retake this Pull Request? What else is required for the merge? I recently updated it to align with the main branch.

It looks like integration with OSD is failing: https://github.com/opensearch-project/oui/actions/runs/7378461865/job/20073761573?pr=1135#step:9:207

We would want to fix that before merging.

BigSamu · 2024-01-02T18:48:08Z

Can we retake this Pull Request? What else is required for the merge? I recently updated it to align with the main branch.

It looks like integration with OSD is failing: https://github.com/opensearch-project/oui/actions/runs/7378461865/job/20073761573?pr=1135#step:9:207

We would want to fix that before merging.

Yeah, I saw that. But is this failure related to something other than our PR? I understand those checks with integration tests to OSD were an issue on your side, didn't it?

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

… upgraded in this PR Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BigSamu · 2024-01-03T00:56:09Z

Can we retake this Pull Request? What else is required for the merge? I recently updated it to align with the main branch.

It looks like integration with OSD is failing: https://github.com/opensearch-project/oui/actions/runs/7378461865/job/20073761573?pr=1135#step:9:207
We would want to fix that before merging.

Yeah, I saw that. But is this failure related to something other than our PR? I understand those checks with integration tests to OSD were an issue on your side, didn't it?

@BSFishy,

At last, I found the problem. The typescript dependencies listed below through an error in the OSD integration tests when they are installed as devDependenceis

"@types/chroma-js": "^2.4.0",
"@types/react-beautiful-dnd": "^13.1.3",
"@types/react-input-autosize": "^2.2.1",
"@types/react-window": "^1.8.5",

This is weird. Should not these dependencies only be used in development? Why are they required on runtime? Maybe there is a problem with the build process during the OSD integration tests.

BSFishy · 2024-01-04T17:11:31Z

This is weird. Should not these dependencies only be used in development? Why are they required on runtime? Maybe there is a problem with the build process during the OSD integration tests.

Yeah this is super weird to me too. I know OSD has some non-standard bootstrap procedures which may be causing these issues. I think @AMoo-Miki might have some more insights into how OSD bootstrap works and why this may be an issue

BigSamu · 2024-01-04T19:09:16Z

This is weird. Should not these dependencies only be used in development? Why are they required on runtime? Maybe there is a problem with the build process during the OSD integration tests.

Yeah this is super weird to me too. I know OSD has some non-standard bootstrap procedures which may be causing these issues. I think @AMoo-Miki might have some more insights into how OSD bootstrap works and why this may be an issue

So, as you said, @BSFishy, @AMoo-Miki reply to this inquiry in our internal chats. I added his response below to update everyone. So I think we are good to go now, or not?

From Miki:

"OSD and plugins don’t ONLY use OUI’s disted artifacts and hence, OSD type-checks OUI during build. If OUI doesn’t have its own deps installed when placed in OSD, the type-check and build will fail. So, either OUI will have to redist those types in its own d-ts, or it has to rely on the defs it finds. They chose to use the defs it finds and to avoid conflicts with any types that OSD might have, they just added these as regular deps"

BSFishy

It may be possible to move these @types/ dependencies into dev dependencies, but this PR fulfills its goals.

If I'm not mistaken, the issue is that OUI exposes types from these dependencies in its own public API. This means that OSD will need to be able to reference these types as well. So if we're able to trick Typescript into inlining these types in the .d.ts file, we could presumably get rid of these dependencies. This could be taken up in a followup issue

AMoo-Miki · 2024-01-11T19:51:33Z

package.json

@@ -212,7 +209,6 @@
    "jest": "^24.1.0",
    "jest-cli": "^24.1.0",
    "moment": "^2.29.4",
-    "moment-timezone": "^0.5.41",


moment-timezone enriches moment and needs to stay.

@AMoo-Miki dependency added back!

I'd keep the ^ that was there before

CHANGELOG.md

resolved

joshuarrrr

It looks like this branch has some of the flex related changes from another PR.

joshuarrrr · 2024-01-13T01:51:21Z

package.json

@@ -212,7 +209,6 @@
    "jest": "^24.1.0",
    "jest-cli": "^24.1.0",
    "moment": "^2.29.4",
-    "moment-timezone": "^0.5.41",


I'd keep the ^ that was there before

joshuarrrr · 2024-01-13T01:52:58Z

src-docs/src/views/flex/flex_basis_string.tsx

@BigSamu It looks like this PR branch got mixed up with the flex changes from another branch. This PR should just have 3 files, right (package.json, yarn.lock, and CHANGELOG.md)?

Ouuuu, you're right!!! My god. Just fixed! Thanks for the heads up @joshuarrrr

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* removing/reassigning unused/wrong-classified runtime dependencies Signed-off-by: Samuel Valdes Gutierrez <[email protected]> * Remove moment-timezone dependency Signed-off-by: Johnathon Bowers <[email protected]> * Remove dependency: eslint-plugin-jest Signed-off-by: Willie Hung <[email protected]> * updating CHANELOG.md with PR 1135 Signed-off-by: Samuel Valdes Gutierrez <[email protected]> * Update CHANGELOG.md typo Co-authored-by: Matt Provost <[email protected]> Signed-off-by: Samuel Valdes Gutierrez <[email protected]> * updating yarn.lock with last changes coming from main (other updates in oui) Signed-off-by: Samuel Valdes Gutierrez <[email protected]> * udpating yarn.lock file Signed-off-by: Samuel Valdes Gutierrez <[email protected]> * downgrading @types/react-window v1.8.7 to v1.8.5 Signed-off-by: Samuel Valdes Gutierrez <[email protected]> * downgrade version of some typescript dependencies that shouldn't been upgraded in this PR Signed-off-by: Samuel Valdes Gutierrez <[email protected]> * adding dev dependency moment-timezone adn update yarn.lock file Signed-off-by: Samuel Valdes Gutierrez <[email protected]> * moving dev dependencies to running dependencies Signed-off-by: Samuel Valdes Gutierrez <[email protected]> --------- Signed-off-by: Samuel Valdes Gutierrez <[email protected]> Signed-off-by: Johnathon Bowers <[email protected]> Signed-off-by: Willie Hung <[email protected]> Co-authored-by: Johnathon Bowers <[email protected]> Co-authored-by: Willie Hung <[email protected]> Co-authored-by: Matt Provost <[email protected]> Co-authored-by: Josh Romero <[email protected]> (cherry picked from commit 335ca98) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* removing/reassigning unused/wrong-classified runtime dependencies * Remove moment-timezone dependency * Remove dependency: eslint-plugin-jest * updating CHANELOG.md with PR 1135 * Update CHANGELOG.md typo * updating yarn.lock with last changes coming from main (other updates in oui) * udpating yarn.lock file * downgrading @types/react-window v1.8.7 to v1.8.5 * downgrade version of some typescript dependencies that shouldn't been upgraded in this PR * adding dev dependency moment-timezone adn update yarn.lock file * moving dev dependencies to running dependencies --------- (cherry picked from commit 335ca98) Signed-off-by: Samuel Valdes Gutierrez <[email protected]> Signed-off-by: Johnathon Bowers <[email protected]> Signed-off-by: Willie Hung <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Johnathon Bowers <[email protected]> Co-authored-by: Willie Hung <[email protected]> Co-authored-by: Matt Provost <[email protected]> Co-authored-by: Josh Romero <[email protected]> Co-authored-by: Miki <[email protected]>

BigSamu and others added 6 commits October 24, 2023 15:57

removing/reassigning unused/wrong-classified runtime dependencies

016e206

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

Remove moment-timezone dependency

703906b

Signed-off-by: Johnathon Bowers <[email protected]>

Remove dependency: eslint-plugin-jest

c9be4e1

Signed-off-by: Willie Hung <[email protected]>

Merge branch 'main' into clean/unused-packages

fbf47a1

Merge branch 'clean/unused-packages' of https://github.com/BigSamu/oui …

0c3c189

…into clean/unused-packages

Merge branch 'opensearch-project:main' into clean/unused-packages

f93db33

BigSamu requested review from seanneumann, AMoo-Miki, rednaksi91, KrooshalUX, BSFishy, bandinib-amzn, ashwin-pc, joshuarrrr, andreymyssak and SergeyMyssak as code owners November 1, 2023 16:01

github-actions bot added the first-time-contributor label Nov 1, 2023

BigSamu mentioned this pull request Nov 1, 2023

Audit unused packages #595

Open

BigSamu added 2 commits November 1, 2023 16:07

updating CHANELOG.md with PR 1135

abe9415

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

Merge branch 'clean/unused-packages' of https://github.com/BigSamu/oui …

d94fb3a

…into clean/unused-packages

BigSamu changed the title ~~[CLEAN] Audit unused dependencies in OUI~~ [OSCI][CLEAN] Audit unused dependencies in OUI Nov 1, 2023

BSFishy reviewed Nov 2, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

BSFishy added the OSCI label Nov 2, 2023

JohnathonBowers reviewed Nov 2, 2023

View reviewed changes

Update CHANGELOG.md typo

ab7e16e

Co-authored-by: Matt Provost <[email protected]> Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

joshuarrrr added 3 commits November 14, 2023 09:51

Merge branch 'main' into clean/unused-packages

7d29d26

Merge branch 'main' into clean/unused-packages

20547f7

Merge branch 'main' into clean/unused-packages

7968509

BigSamu added 3 commits January 1, 2024 12:36

Merge branch 'main' into clean/unused-packages

664e414

udpating yarn.lock file

18ee423

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

Merge branch 'clean/unused-packages' of https://github.com/BigSamu/oui …

a18a115

…into clean/unused-packages

BigSamu added 2 commits January 2, 2024 18:07

downgrading @types/react-window v1.8.7 to v1.8.5

95dca1e

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

downgrade version of some typescript dependencies that shouldn't been…

7c08704

… upgraded in this PR Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BSFishy approved these changes Jan 4, 2024

View reviewed changes

AMoo-Miki reviewed Jan 11, 2024

View reviewed changes

AMoo-Miki previously requested changes Jan 11, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

BigSamu requested a review from AMoo-Miki January 12, 2024 15:02

joshuarrrr requested changes Jan 13, 2024

View reviewed changes

BigSamu force-pushed the clean/unused-packages branch 2 times, most recently from fcc6fa9 to 7c08704 Compare January 13, 2024 13:23

BigSamu added 3 commits January 13, 2024 10:44

adding dev dependency moment-timezone adn update yarn.lock file

f8a5d12

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

moving dev dependencies to running dependencies

7d6a310

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

Merge branch 'main' into clean/unused-packages

5184ea9

BigSamu requested a review from joshuarrrr January 13, 2024 15:11

joshuarrrr approved these changes Jan 18, 2024

View reviewed changes

joshuarrrr merged commit 335ca98 into opensearch-project:main Jan 18, 2024
15 checks passed

BigSamu deleted the clean/unused-packages branch January 18, 2024 18:04

joshuarrrr added the backport 1.x label Feb 8, 2024

opensearch-trigger-bot bot mentioned this pull request Feb 8, 2024

[Backport 1.x] [OSCI][CLEAN] Audit unused dependencies in OUI #1247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OSCI][CLEAN] Audit unused dependencies in OUI #1135

[OSCI][CLEAN] Audit unused dependencies in OUI #1135

BigSamu commented Nov 1, 2023 •

edited

Loading

BSFishy left a comment

JohnathonBowers left a comment

BigSamu commented Nov 4, 2023

willie-hung commented Nov 6, 2023

BigSamu commented Jan 1, 2024 •

edited

Loading

BSFishy commented Jan 2, 2024

BigSamu commented Jan 2, 2024

BigSamu commented Jan 3, 2024

BSFishy commented Jan 4, 2024

BigSamu commented Jan 4, 2024

BSFishy left a comment •

edited

Loading

AMoo-Miki Jan 11, 2024

BigSamu Jan 11, 2024

joshuarrrr Jan 13, 2024

joshuarrrr left a comment

joshuarrrr Jan 13, 2024

joshuarrrr Jan 13, 2024

BigSamu Jan 13, 2024

[OSCI][CLEAN] Audit unused dependencies in OUI #1135

[OSCI][CLEAN] Audit unused dependencies in OUI #1135

Conversation

BigSamu commented Nov 1, 2023 • edited Loading

Description

Issues Resolved

Check List

BSFishy left a comment

Choose a reason for hiding this comment

JohnathonBowers left a comment

Choose a reason for hiding this comment

BigSamu commented Nov 4, 2023

willie-hung commented Nov 6, 2023

BigSamu commented Jan 1, 2024 • edited Loading

BSFishy commented Jan 2, 2024

BigSamu commented Jan 2, 2024

BigSamu commented Jan 3, 2024

BSFishy commented Jan 4, 2024

BigSamu commented Jan 4, 2024

BSFishy left a comment • edited Loading

Choose a reason for hiding this comment

AMoo-Miki Jan 11, 2024

Choose a reason for hiding this comment

BigSamu Jan 11, 2024

Choose a reason for hiding this comment

joshuarrrr Jan 13, 2024

Choose a reason for hiding this comment

joshuarrrr left a comment

Choose a reason for hiding this comment

joshuarrrr Jan 13, 2024

Choose a reason for hiding this comment

joshuarrrr Jan 13, 2024

Choose a reason for hiding this comment

BigSamu Jan 13, 2024

Choose a reason for hiding this comment

BigSamu commented Nov 1, 2023 •

edited

Loading

BigSamu commented Jan 1, 2024 •

edited

Loading

BSFishy left a comment •

edited

Loading