-
Notifications
You must be signed in to change notification settings - Fork 72
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
[CCI] Update dependencies #609
[CCI] Update dependencies #609
Conversation
2beec92
to
d16db80
Compare
@BSFishy At the moment |
84424d9
to
fe93252
Compare
I haven't had a chance to have a look at all |
@andreymyssak Thanks for the very clear and detailed PR description. Makes reviewing this a lot easier. Lets keep reorganizing the dependencies for another PR since its easier to review and validate smaller units of change at a time. |
This is great @andreymyssak. We'll get this reviewed! |
Me and @SergeyMyssak updated the resolutions, we decided not to use
What do you think of this approach? |
Thanks for the work, this looks great. When I wrote #594, I meant for it to be a discovery/analysis task, rather than a task calling for changes. Specifically, I was looking to see what dependencies could be trivially updated (no or minimal changes), which ones could be updated with some work, which ones can or should be removed, which ones should be replaced with other dependencies, etc. However, it looks like a lot of that analysis and discovery already happened in doing the work for this PR. I'd still like for that discovery to be easily accessible, so if you wouldn't mind, do you think you could add a comment on that issue with the analysis and discovery you did for this PR?
As @ashwin-pc mentioned, this should be done in a separate PR. Keeping PRs as small as possible makes it a lot easier to understand and review quickly.
That's perfect. Makes it extremely easy to see which resolutions we need to take a look at when updating one of those packages |
15c83cd
to
6974d07
Compare
Discovery/analysis based on this PR added to the issue |
Thanks for the analysis. 2 final important questions before reviewing: are there any breaking changes on the engineering side with these changes (i.e. has the public API changed in a breaking way), and are there any changes on the UX side (i.e. will anything change in terms of visuals or the way the components are used by the end user)? |
Component properties and their UI have not changed. The only thing that may have some hidden effect on UX is the updated |
6974d07
to
bed4aa0
Compare
Should we open a follow-up issue to look into alternatives for these?
This sounds like it may warrant a follow-up issue |
I'm currently looking into using Vite instead of Webpack and being ESM module native. Doesn't necessarily change anything now, but wanted to give that context |
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.
@andreymyssak While I started to review this PR, I realized that safely validating the change in a reasonable amount of time would be nearly impossible since there are way too many changes because of too many dependency updates happening at the same time. While I dont expect you to raise an individual PR for each change (since that would be quite tedious and unnecessary), can you split this PR into smaller PR's of 2 kinds.
- Group all dependency updates that do not change any code into one
- Separate PRs for anything that modifies more than 2 code files. e.g. a separate PR for the SVGO update since it updates a ton of icons.
This way we will be able to get to this PR faster and accept your contribution sooner. The main reason for this is that while there are CI tests that in theory should give us confidence in the change., we dont have any integ tests that make sure that this change does not affect the library in a functional way and that has to be done manually at the moment.
i18ntokens.json
Outdated
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.
Why are these values changing for a dependency update PR?
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.
The changes in i18ntokens.json
happened after updating the tabbable
dependency (I just ran the extract-i18n-strings
script after updating it).
I don't know the purpose of this script, but if you run it now in the main
branch, this file will be updated because there were some changes in super_date_picker
file and extract-i18n-strings
script was not run.
"@types/refractor": "^3.0.0", | ||
"@types/resize-observer-browser": "^0.1.5", | ||
"@types/vfile-message": "^2.0.0", |
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.
why has this been removed?
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.
why has this been removed?
@types/[email protected]: This is a stub types definition. vfile-message provides its own type definitions
@@ -43,84 +43,93 @@ | |||
"url": "https://github.com/opensearch-project/oui.git" | |||
}, | |||
"resolutions": { | |||
"**/trim": "0.0.3", | |||
"**/axios": "^0.21.1", | |||
"**/ansi-html": "^0.0.8", |
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.
I dont see this resolution anymore
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.
I dont see this resolution anymore
"**/trim" -> "remark-parse/trim": "0.0.3"
"**/axios" -> "codesandbox/axios": "^0.21.1"
"**/ansi-html" is not used
a9c7297
to
ba02d3b
Compare
The way you suggested to break down dependency updates is really good and makes it much easier to review changes. Below you can find PRs that make some changes to the code:
|
ba02d3b
to
602b59c
Compare
Co-authored-by: Sergey Myssak <[email protected]> Signed-off-by: Andrey Myssak <[email protected]>
602b59c
to
ffa85f6
Compare
@andreymyssak My understanding is that we can close this in favor of the other PRs. But if I misunderstood or this needs to be reopened, please do so. |
@joshuarrrr This PR was large, so I moved some of the dependencies into separate PRs and left the ones in the current PR that don't make any changes to the code. I don't have a "Reopen pull request" button, should I then make a new PR? |
Description
Update dependencies that do not lead to breaking changes
yo
dependency will be updated in the scope of #599webpack
dependencies will be updated in the scope of #587Results of
yarn install
Notes
glob
dependency higher than v8.1.0 requiresNode >= 16
get-port
dependency higher than v5.1.1 works only with ESM modulesfakerjs
is no longer supported, I suggest migrating to@faker-js/faker
chalk
dependency higher than v4.1.2 works only with ESM modulesreact-docgen-typescript
dependency higher than v1.22.0 requirestypescript >= 4.3.x
puppeteer
dependency cannot be updated to v19 because@axe-core/puppeteer
has specifiedpuppeteer v18
as a peer dependency.react-focus-on
dependency higher than v3.5.0 causes the following errorError: Error: Error: Uncaught [Error: aria-hidden]
in the unit tests.react-ace
dependency has not been updated because the component in which it is used (OuiCodeEditor
) is outdated and will be removed in future releases according to the documentation.Dependencies that require much effort to update are better moved to separate tasks:
eslint
+prettier
react
(it is necessary to migrate toreact-testing-library
)typescript
babel
+jest
postcss
unified
+rehype-*,
remark-*
,vfile
,mdast-util-to-hast
react-view
Issues Resolved
#594
Check List
yarn lint
yarn test-unit
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.