Upgrade redux toolkit to 2.2.2 #178986
Upgrade redux toolkit to 2.2.2 #178986
Conversation
Zacqary
added
release_note:skip
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
|
Pinging @elastic/response-ops (Team:ResponseOps) |
There was a problem hiding this comment.
Code seems to LGTM, but when I try to build this locally the bootstrap script fails. Is this happening for anyone else?
[bazel] ERROR: /Users/jk/git/justinkambic/kibana/packages/kbn-ui-shared-deps-npm/BUILD.bazel:72:12: Action packages/kbn-ui-shared-deps-npm/shared_built_assets failed: (Exit 1): webpack-cli.sh failed: error executing command bazel-out/host/bin/external/npm/webpack-cli/bin/webpack-cli.sh --config packages/kbn-ui-shared-deps-npm/webpack.config.js --output-path ... (remaining 3 arguments skipped)
I deleted basically everything and tried doing a fresh install a few different ways and it's failing every time.
|
Please note that for previous updates of For example in the file This PR #166367 lists the redux related packages that are part of the shared bundle: |
I'm seeing the same thing, I can't test the PR at the moment. This is the error I'm getting when running |
|
There's a if (cache?.has(value))is not valid Javascript Trying to figure out if this is a problem on our end or on Redux's end. |
|
Okay actually it turns out they added optional chaining to ECMAScript. Our build tool didn't get the memo, though. Looking into this. |
|
Turns out EUI ran into this issue as well with another package, and it's because Kibana is still on Webpack 4: #166676 (comment) I've opened this issue on Redux Toolkit to track, because they explicitly were hoping to achieve Webpack 4 compatibility: reduxjs/redux-toolkit#4282 May need to put this upgrade on hold till Kibana is on Webpack 5, depending on what the Redux team says. EDIT: Redux maintainers are fixing this today |
|
Security solution makes use of the maxSize option passed by reselect to the lruMemoize object that reselect 4.x uses by default, and now in 5.x that shouldn't be needed/might blow up once we move to redux-toolkit 2.2.2, as it uses weakMapMemoize by default instead, which does not support maxSize. Will need to test these parts of security solution once everything is up to date. See the reselect 5.0 changelog https://github.com/reduxjs/reselect/releases/tag/v5.0.1 for more, not sure if it will fail silently or just ignore the option or what right now. |
Zacqary
changed the title
|
@kqualters-elastic Understood, I'll do my best to test this myself but some confirmation from Security that this is good to go would be great. 2.2.2 is merged in this branch now. @justinkambic @PhilippeOberti Bootstrapping works now with the latest commit if y'all want to test again |
|
Only 1 file needs to be changed if so https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/common/store/sourcerer/selectors.ts I would expect the reselect types to catch this tbh, but maybe not. Will manually test soon. |
|
Tricky PR, thanks for picking it up and making good progress! At the moment we still have |
botelastic
bot
added
the
Team:obs-ux-infra_services
# Conflicts: # package.json # yarn.lock
|
@justinkambic Synthetics is fixed, there was an @PhilippeOberti Fixed security as well, same |
|
@walterra Any reason to use |
|
@Zacqary What I saw from the dependency requirements is that |
⏳ Build in-progress, with failuresFailed CI Steps
Test Failures
History
To update your PR or re-run it, just comment with: cc @Zacqary |
Ok I will smoke test this for you soon and if Synthetics is un-busted I'll approve for my team. |
There was a problem hiding this comment.
doing some smoke testing and I'm finding a few issues:
-
clicking on the
Analyze eventicon in the alerts table throws an error. It seems that theanalyzerstore isn't correctly initialized. Theanalyzerstate stay undefined, which then throws error in the selectors. I think it might be coming from this file but I'm not familiar with the code. Maybe @kqualters-elastic or @christineweng can help take a look?
-
I'm also seeing that the alerts page filters are completely broken. Clicking on them doesn't do anything
This is onmain:
https://github.com/elastic/kibana/assets/17276605/b193e2f7-37ca-4f93-a079-6839db76efcd
This is on this branch:
https://github.com/elastic/kibana/assets/17276605/e7105b12-0db7-4054-b701-86cf0c14af0c
I'm also unfamiliar with this code, so maybe @logeekal will be able to help?
- finally I see hundreds (thousands) of these warnings while navigating the app.
While these are just warnings, they're going to be extremely annoying for developers who want to `console.log` things, as they're going to be buried under the amount of warnings...
I need to continue smoke testing the app as I'm afraid there might be a lot of these happening...
I'm not sure what's the path we should take here. Do you want us (by us I mean Threat Hunting Investigations team) to help out and contribute to this PR by pushing commits fixing some of these things?
|
@PhilippeOberti I think at least some of that is mentioned (along with the fix) in the immer 10.0 release notes breaking changes bit, specifically this part: https://github.com/immerjs/immer/releases/tag/v10.0.0 |
That would be a big help, thank you. I was hoping this upgrade would be a simpler process based on consumers of Redux Toolkit, but a total KIbana Redux v4-v5 migration is a larger task that's difficult for me to do in parallel with ResponseOps's new Redux project. |
|
@Zacqary are there specific features y'all need in v5, or can you get started using v4, and just be mindful of breaking changes and work around any of those possibly? |
|
Not only do we need the app to run, but we should probably verify that upgrading reselect didn't introduce a memory leak in all of the places that make use of that library. This line in the release notes linked above 👀 : "Also, note that an "infinite cache size" from one point of view can be considered a "memory leak" for another point of view" ha. And the fun part about reselect is, if somewhere in kibana we are using a selector wrong and nothing is actually being memoized, calling it again and again will return the desired value still, and there's no apparent logic error. Before that would just be waste that gets eventually garbage collected I think, but now it's a memory leak 🥲 we recently fixed a few selectors in security solution that were failing open like this. |
|
@kqualters-elastic We can absolutely get started in v4, this PR is primarily to deal with the tech debt of being behind. We're using Redux primarily because it's a highly documented architecture with best practices that we don't have to write and maintain in-house, so we're concerned about being a version behind the most easily accessible docs. |
|
Before we upgrade any of this, I think we need to do the following: https://redux.js.org/usage/migrating-to-modern-redux |
Summary
Updates
@reduxjs/toolkitto2.2.12.2.2 in preparation to use it in the Rule Form v2. It looks like breaking changes from 1.x to 2.x don't affect us.Also updates core Redux packages to the same dependencies required by Toolkit.
Affected codeowners: