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

refactor: remove immutable from 'config' state slice #4776

Closed

Conversation

smashercosmo
Copy link
Contributor

Summary

Continue with #3853

Test plan

Run existing tests

@smashercosmo smashercosmo requested a review from a team December 28, 2020 18:12
@smashercosmo
Copy link
Contributor Author

Alright. Have to admit, this was a bit harder than previous refactorings and unfortunately required a lot of changes to be made in different places. It's 99% done, but still requires some polishing and thorough review. But at least all unit test are passing and TypeScript is mostly happy, except for some tricky places, which I still don't know how to handle properly (I'll mention all of them in comments).
Also I had to copypaste some utility functions from collections to config in order to avoid also making changes to collections.
And also I had to copy proper types from index.d.ts to types/redux.ts (don't know what would be the best strategy in this case).

@smashercosmo smashercosmo force-pushed the remove-immutable-from-config branch 9 times, most recently from 7c9bcd6 to f35ee6f Compare December 28, 2020 23:57
@smashercosmo smashercosmo force-pushed the remove-immutable-from-config branch 2 times, most recently from bcbf658 to 01651ab Compare December 29, 2020 01:30
@@ -655,9 +655,12 @@ const evaluateFolder = (
collection.get(folderKey)!,
entryMap,
collection,
// TODO can't figure out how to handle this
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore
// @ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to config type media_folder can be undefined

@@ -700,9 +703,12 @@ const evaluateFolder = (
collection.get(folderKey)!,
entryMap,
collection,
// TODO can't figure out how to handle this
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore
// @ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to config type media_folder can be undefined

if (camel in field) {
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore
// @ts-ignore seems like it's impossible to make TS happy in this case
field[snake] = field[camel];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't come up with proper typing :(

@erezrokah erezrokah added the type: chore work needed to keep the product and development running smoothly label Dec 29, 2020
@smashercosmo
Copy link
Contributor Author

There is no need to check this PR right now. I'm gonna send several more atomic PRs to make merging this one easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants