Skip to content
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

Update all webui dependencies and refactor breaking changes #6327

Merged
merged 24 commits into from
Jul 4, 2023

Conversation

kschiffer
Copy link
Member

@kschiffer kschiffer commented Jun 14, 2023

Summary

This PR updates (almost) all javascript dependencies to the latest version applies many refactors to fix breaking changes.

Changes

  • Update all dependencies except react, which I will do separately
  • Apply many refactors to fix breaking changes, biggest of which were:
    • react-leaflet
    • react-router v6
    • yup
  • Apply new formatting to align with new eslint version
  • Apply webpack configuration changes to work with newer plugin versions
  • Removing connected-react-router
  • Removing many occurrences of higher-order-components (mainly connect(), withRequest(),withFeatureRequirement(), withBreadcrumbs()) and replacing them with respective hooks.

Testing

Tested a lot manually and end-to-end tests are passing.

Notes for Reviewers

Please see the detailed account in my comment below.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@kschiffer kschiffer added c/console This is related to the Console security This is important for security dependencies Pull requests that update a dependency file ui/web This is related to a web interface tooling Development tooling labels Jun 14, 2023
@kschiffer kschiffer added this to the v3.26.2 milestone Jun 14, 2023
@kschiffer kschiffer self-assigned this Jun 14, 2023
@github-actions github-actions bot removed tooling Development tooling security This is important for security c/console This is related to the Console labels Jun 14, 2023
@kschiffer kschiffer mentioned this pull request Jun 19, 2023
5 tasks
@kschiffer kschiffer force-pushed the fix/webui-dependency-updates branch from e91742e to 7454ed9 Compare June 29, 2023 15:22
@github-actions github-actions bot added the tooling Development tooling label Jun 29, 2023
@kschiffer kschiffer force-pushed the fix/webui-dependency-updates branch from 7454ed9 to 6a3105a Compare June 30, 2023 13:25
@KrishnaIyer KrishnaIyer modified the milestones: v3.26.2, v3.27.0 Jun 30, 2023
@kschiffer kschiffer force-pushed the fix/webui-dependency-updates branch 2 times, most recently from 46570d4 to 1aa6f60 Compare July 2, 2023 17:41
@kschiffer kschiffer marked this pull request as ready for review July 2, 2023 17:41
@kschiffer kschiffer force-pushed the fix/webui-dependency-updates branch 2 times, most recently from 826cba1 to e2c296b Compare July 3, 2023 09:50
@adriansmares adriansmares changed the base branch from v3.26 to v3.27 July 3, 2023 10:56
@kschiffer
Copy link
Member Author

Here's a more detailed breakdown of what has been done in this PR:

  • The vast majority of the refactors throughout the codebase have to do with the update to react-router-dom v6, which works exclusively with hooks to obtain route information. Consequently, all usages of routes and routing had to be refactored to not use passed route parameters, but obtain such parameters from the new useParams() and similar hooks.
  • I decided to remove connected-react-router entirely because it never served a real purpose for us to pipe routing information through our global state and it only made implementation more complicated while adding additional bundle weight and vulnerability area. As a result all redux facilitated route changes (using dispatch(push()/replace())) had to be refactored to use react-router's useNavigate() directly instead.
  • All in all, this was a good opportunity to completely get rid of (almost) all HOCs, such as connect(), withRequest(), withBreadcrumbs() and proceeding to use hooks instead.
    • This also meant that the classic file structure we had of index.js, component.js, connect.js is no longer purposeful and instead, we can usually merge everything into a single index.js file.
    • In case of having to fetch initial data for a view, it is then required to use an inner and outer functional component, where the outer component fetches all required data and passes it down to the inner component. This is to avoid scenarios of running component logic that depends on fetched data without having such data ready initially. (This is somewhat similar to what higher-order components did)
  • With the update to yup 1.0.0, it was necessary to refactor some validation schemas that used .when() to use arrays of parameters instead of spreading.
  • To make the fetch table work again without HOCs, I had to use createSelector() hooks which take care of composing objects off of multiple selectors, to avoid unnecessary rerenders. Perspectively though, <FetchTable /> definitely needs to be rewritten entirely to improve maintainability and its usage.
  • I also rewrote <ApiKeysForm /> and <CollaboratorsForm /> to be easier to use and maintain, while converting them to functional components and moving the logic of selecting correct selectors per entity to the containers instead of passing them on consumer level.
  • I've also updated Cypress to the latest version, which required a bunch of updates to the spec to update deprecated instructions and rewriting the whole configuration setup to match the new format. Luckily, the tests run much faster now (at least locally)
  • It was not possible unfortunately to upgrade babel-plugin-react-intl because it changed message collection in a way that is incompatible to our pipeline. I've spent quite some time researching ways to resolve this but had to give up for the time being.
  • Similarly, it was not possible to update redux-actions to 3.0.0 because it would use ES module features that are incompatible with our unit-testing framework jest, even when transpiling via babel. This might be resolved in upcoming versions.

All in all the refactor became quite huge because there were so many components affected by the router update, but functionally there are no changes and the end-to-end test run through, so I think we can trust this quite a bit but I would still suggest scanning the diff to get a gist of what these refactors are about.

cc @ryaplots @mjamescompton @adriansmares

@kschiffer kschiffer force-pushed the fix/webui-dependency-updates branch 2 times, most recently from 4f37a5d to fce6bc2 Compare July 3, 2023 12:08
pkg/webui/console/views/admin/index.js Outdated Show resolved Hide resolved
@kschiffer kschiffer force-pushed the fix/webui-dependency-updates branch from 68580f4 to 3e44f75 Compare July 3, 2023 16:04
@kschiffer kschiffer force-pushed the fix/webui-dependency-updates branch from 3e44f75 to 3fb25c1 Compare July 3, 2023 16:22
@kschiffer kschiffer requested a review from ryaplots July 3, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file tooling Development tooling ui/web This is related to a web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants