-
Notifications
You must be signed in to change notification settings - Fork 53
Fix ts 1 #5768
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
base: master
Are you sure you want to change the base?
Fix ts 1 #5768
Conversation
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.
Love that this is getting fixed!
| <SearchPageResults | ||
| onKeyDown={handleKeyDown} | ||
| onSelect={handleSelect} | ||
| onSelect={handleSelect as (arg0: SearchResult) => void} |
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.
Does ts not complain about param names such as arg0?
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.
not quite sure, a little hard to find in the about 1000 types script error, but I don think so. Also it looks like some other function use swell
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 think it does not complain. Any ways i think a better approach to solving ts errors such as this is to instead fix the type of onSelect prop or declare the handleSelect to match the expected type instead of casting it like this.
package.json
Outdated
| "devDependencies": { | ||
| "@types/redux": "^3.6.31", | ||
| "redux": "^5.0.1" |
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.
Do you need this aswell as "react-redux" in lego-webapp/lego-webapp/package.json
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 did get some error someplace, and this fixed the issue, I don't know why
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 think the errors being fixed by this are those relating to usages of AnyAction from redux because it has been deprecated. I think a better fix is to actually fix the errors instead of using what i guess is old types as a work around.
See redux docs: https://redux-toolkit.js.org/usage/migrating-rtk-2#anyaction-deprecated-in-favour-of-unknownaction
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.
Never mind i think the error is related to that it cant find the redux model which explains why adding it resolves them. However I think that adding this is still wrong approach since we already use react-redux. I can look a bit more into it
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 think the proper way to solve these errors are to delete all imports from redux as we dont need it and instead import AnyAction from @reduxjs/toolkit. Adding a whole new package (except purely type packages) i think rarely is the correct apporach for fixing types.
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.
AnyAction is still deprecated in favor of UnknownAction but I notice that updating this requires more work and this is not a ts error anyways so I think its okay to use AnyAction for this pr, just change the import
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.
🪨
Co-Authored-By: isak <[email protected]>
c88c01c to
1898db2
Compare
1898db2 to
a127dcf
Compare
Description
Lego has way too many ts issues. This is an attempt to fix some of them. I have tried checking different pages and nothing looks of, but please try running the program,
Result
Testing
Please describe what and how the changes have been tested, and provide instructions to reproduce if necessary.
I clicked around on different pages
Resolves ABA-1587