Core dependency upgrades

[aristidesstaffieri]

Closes #1824

Major dependency upgrade for -
react
react-router
typescript
styled-components
clsx
@testing-library/*
@reduxjs-toolkit

Notable changes -
Improved store types
new routing API from react-router v7, significant changes to the routing paradigm and architecture, mostly found in Router.tsx but also impacts all nested routes.

Regressions -
Send/Swap tests are not easily compatible with these changes due to the changes to nested routes. I've commented and detailed the situation and potential ways to revive them in the change set diffs for the tests.

[socket-security]

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert	Package	Note	Source	CI
Deprecated	npm/@types/history@5.0.0	Reason: This is a stub types definition. history provides its own type definitions, so you do not need this installed.	extension/package.json	⚠︎
Deprecated	npm/@types/testing-library__jest-dom@6.0.0	Reason: This is a stub types definition. @testing-library/jest-dom provides its own type definitions, so you do not need this installed.	extension/package.json	⚠︎
Deprecated	npm/@types/qrcode.react@3.0.0	Reason: This is a stub types definition. qrcode.react provides its own type definitions, so you do not need this installed.	extension/package.json	⚠︎

View full report↗︎

Next steps

What is a deprecated package?

The maintainer of the package marked it as deprecated. This could indicate that a single version should not be used, or that the package is no longer maintained and any new vulnerabilities will not be fixed.

Research the state of the package and determine if there are non-deprecated versions that can be used, or if it should be replaced with a new, supported solution.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

• @SocketSecurity ignore npm/@types/history@5.0.0
• @SocketSecurity ignore npm/@types/testing-library__jest-dom@6.0.0
• @SocketSecurity ignore npm/@types/qrcode.react@3.0.0

[aristidesstaffieri]

Because of the changes to nested routes in v7, we need to only use the path after the base path from the nested structure. In order to keep the existing mappings in routes.ts, we can split away the base path and pass the page specific path to each nested route.

This pattern is used for all nested routes but only commented about here.

[piyalbasu]

Can we create a helper for this where we pass in the nested path and the base path?

getNestedRoute(ROUTES.accountMigratonReviewMigration, ROUTES.accountMigration)

[aristidesstaffieri]

What do you have in mind?
It's not always the same prefix or the same number of prefix paths so I think the helper would essentially just be a wrapper around String.split.

[piyalbasu]

Oh gotcha - it looked like the nested route was always ${baseRoute}/{subRoute}, where you're always just extracting subRoute. The nested routes are generally set up that way, but I see we actually have another level of nesting on some of them which makes this non-standard.

Hm, this does feel a little messy having these .split's at the top. Maybe the real fix here is just moving away from this nested router structure 🤔 But that's maybe a new ticket

For now, to avoid duplicating the same string a bunch of times, can we at least do:

const baseRoute = `${ROUTES.accountMigration}/`;
const review Slug = ROUTES.accountMigrationReviewMigration.split(
    baseRoute,
  )[1];

[aristidesstaffieri]

hmm not sure I see the example, aren't they all different sub paths already?

Yeah there is def a discussion about the routing because the sub routes don't work well for "full screen mode" but yeah I feel that's outside of this scope and would be a significant refactor.

[aristidesstaffieri]

Will pull out the base path into a var here, incoming patch.

[aristidesstaffieri]

this is done in 96ef7f5

[aristidesstaffieri]

I haven't found a way to maintain this test as written. Because of the nested router changes and the way the send/swap component relies on location to render the appropriate copy/components, the test fails to render the correct screen. The top level router hasn't been part of the tests and the changes to the global history make maintaining this pattern difficult.

I think we can rewrite the send/swap components to rely on an injected location from the useNavigate hook while also refactoring the tests in order to render use a test router to mimic the nested routing structure in the full application.

[aristidesstaffieri]

I haven't done this because of the massive change set already, it seemed like this could be broken off into another PR but I am open to suggestions.

[CassioMG]

Agree on breaking this off into another PR, sounds like the best approach for simplicity given the size and complexity of this PR

[aristidesstaffieri]

This is subject to the same details as the SendPayment tests and can be revived using the same solutions.

[aristidesstaffieri]

Same comments as other skipped tests

[aristidesstaffieri]

Our current linting pipeline relies on @stellar/config which is not compatible with the upgraded version of typescript/react. We have to upgrade eslint & friends in that repo before we can use that config in this repo.

[piyalbasu]

Do we know what the lift is on upgrading @stellar/config? I think we should do that first if it's reasonable. If we put it off, we could introduce a bunch of linting errors we'll need to address down the line

[aristidesstaffieri]

I think it requires a significant rewrite of the config but not sure on the t-shirt size of it. Open to suggestions here but it seemed like doing this could introduce quite a few more changes to this already massive PR due to the likelihood of rules changing across several major versions of the tooling.

[aristidesstaffieri]

this is a pretty big departure since eslint 9 is wildly different but I think this gets us as close as is reasonable given the changes.

done in commit 9134812

[socket-security]

Report too large to display inline

View full report↗︎

[piyalbasu]

will we need to update the node version in the other GHA's, as well?

[aristidesstaffieri]

Not that I've found, but yeah maybe the build ones would too I can upgrade those to 22.

[aristidesstaffieri]

this is done in add3f0c

[piyalbasu]

just calling some things out as I see them - I'll pull this branch and try to see if this actually has an effect:

I notice we're not passing the props here. Wondering if this will break some views

[aristidesstaffieri]

I don't believe it does because the re-organization and use of the the outlet makes those props no longer needed but would love another glance at that as it departed significantly from the previous patterns.

[piyalbasu]

let's rm this comment

[aristidesstaffieri]

done in d88f696

[CassioMG]

nice!

[CassioMG]

nice catch on those "asset-amount" string tweaks

[CassioMG]

Lgtm, thanks for working on this!