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

exportable case reducers type, bugfix, documentation #290

Merged
merged 19 commits into from
Dec 26, 2019

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Dec 21, 2019

This is what I'm currently working on and it has already gotten a bit bigger than expected 😄

INCLUDED:

@netlify
Copy link

netlify bot commented Dec 21, 2019

Deploy preview for redux-starter-kit-docs ready!

Built with commit 96b26c4

https://deploy-preview-290--redux-starter-kit-docs.netlify.com

@phryneas phryneas changed the title exportable case reducers type exportable case reducers type, bugfix, documentation Dec 21, 2019
@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 21, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 96b26c4:

Sandbox Source
kind-browser-bdfkd Configuration
magical-goldstine-chgke Configuration
rsk-github-issues-example Configuration

}
/**
* The type describing a slice's `reducers` option.
* Also checks itself, so it has to be passed "itself" as it's second option.
Copy link
Collaborator

Choose a reason for hiding this comment

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

wat :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wouldn't have found out about that one myself as it is completely counter-intuitive. But a generic can extend (which essentially just means "match this shape") something that depends on the generic being matched.

So you can write

function something<X extends Restrict<X>>(x: X) {}

and then Restrict can be a type that does some crazy restrictions on the original input object while referencing it.

This is something I learned from @jcalz over on StackOverflow: https://stackoverflow.com/questions/59200095/is-there-a-simpler-way-of-associating-a-key-to-a-value-in-a-mapped-obect?noredirect=1#comment104621839_59200095
Thank you again Joe!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still waiting for that blog post / talk on "all the bizarre stuff I learned working on RTK's TS types" :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, got something in the making :) Meetup somewhere mid-february, maybe I'll also submit it to a few conferences. Also, some ideas to experiment with twitter-content :)

@markerikson
Copy link
Collaborator

Your ability to crank out TS improvements continues to amaze me :)

No specific feedback from me on the changes, but as usual I still don't know quite enough to provide much feedback here anyway.

Do you want to try to get the other type doc changes in here too, or save those for later?

@phryneas
Copy link
Member Author

Your ability to crank out TS improvements continues to amaze me :)

This project is a wonderful learning experience, but it kills time like nothing else. I think i've started this Refactor over four or five times, each time putting in like 3-5 hours and then throwing everything away because it just wasn't working out.
So in the end, it just comes down to trial, error and my determination to bash my head against it until I find out how it works ;)

Do you want to try to get the other type doc changes in here too, or save those for later?

I'm planning on doing everything in this PR, now that I already mixed different things in here.
We'll probably have to wait for microsoft/rushstack#1664 anyways and I need to find out why this doesn't work with TS3.3 again 🤷‍♀️

@phryneas
Copy link
Member Author

So it seems that relying on @internal won't be a possibility for us to exclude certain types from the dts-rollup (although it has a nice documenting effect when reading the source 😄 ). Which means I'll have to replace the export * from ... in index.ts with explicit export statements.
@markerikson would that be okay with you? I believe that might even help bundlers to do tree-shaking, although that's only a gut feeling.

The alternative would be to move the type to a different file where nothing is exported from, but right now I'd say that the type is in the file it belongs to.

@markerikson
Copy link
Collaborator

Yeah, if that's necessary to keep internal types from leaking, we can do that.

@phryneas phryneas mentioned this pull request Dec 23, 2019
@phryneas
Copy link
Member Author

Okay, that's it for today. All that's left is fixing TS3.3 - I hope that's gonna work out, but I'll leave that problem for "future me" 😄

@phryneas
Copy link
Member Author

phryneas commented Dec 24, 2019

So, to make this work with TS <3.4, unfortunately I had to move ValidateSliceCaseReducers out of SliceCaseReducers again, because the generic that extends itself to restrict itself tactic seems not to be working in older TS versions.
This means, we have one more exported type: ValidateSliceCaseReducers.
And I renamed SliceCaseReducerDefintions to just SliceCaseReducers as it was the last chance we had for that before exposing the type 😄

But all in all, given that SliceCaseReducers is now a normal type and we aren't going over that "inferred anoymous PayloadActions type" that was in there that was absolutely confusing, I think this whole refactor still has some good value and we can actually expose those types in good consciousness.

Comment on lines 319 to 351
{
interface GenericState<T> {
data?: T
status: 'loading' | 'finished' | 'error'
}

const createGenericSlice = <
T,
Reducers extends SliceCaseReducers<GenericState<T>>
>({
name = '',
initialState,
reducers
}: {
name: string
initialState: GenericState<T>
reducers: Reducers & ValidateSliceCaseReducers<GenericState<T>, Reducers>
}) => {
return createSlice({
name,
initialState,
reducers: {
start(state) {
state.status = 'loading'
},
success(state: GenericState<T>, action: PayloadAction<T>) {
state.data = action.payload
state.status = 'finished'
},
...reducers
}
})
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what wrapping createSlice would look like now.

@phryneas
Copy link
Member Author

phryneas commented Dec 24, 2019

Oh, seriously? Now TS is acting up, only in 3.4 🙄

Just for the "extend slice" test case.

Draft<GenericState<T>> is not compatible with GenericState<T>. Suure.

At this point, I'll just write that test another way, in the worst case someone might be required to do a cast there (Draft and yet-unresolved Generics have their problems anyways), but seeing this happens only in 3.4 most people won't be affected anyways.

I'll move that test to a different file and omit executing that test in Version 3.4. Honestly, I have no idea on how to actually do this at all in 3.4 and since it works in versions before and after just fine, I guess the current state will be "please update TypeScript to use this". sigh

Might be on to something...

@phryneas
Copy link
Member Author

Yay, we're all green, without dirty hacks.

I was almost rage-quitting there o_O

@phryneas phryneas marked this pull request as ready for review December 24, 2019 15:28
src/createReducer.ts Outdated Show resolved Hide resolved
src/createReducer.ts Outdated Show resolved Hide resolved
src/createSlice.ts Outdated Show resolved Hide resolved
type-tests/files/createSlice.typetest.ts Show resolved Hide resolved
@markerikson
Copy link
Collaborator

No immediate complaints about the TS stuff, but my eyes also mostly glazed over, so take it as you will :)

Saw a couple possible comment typos, suggested a rewording, and it'd be nice to see the "generic slice" example in the docs, but other than that looks okay.

phryneas and others added 5 commits December 24, 2019 20:33
@phryneas
Copy link
Member Author

Added a "Wrapping createSlice" chapter to the TS docs. While I was at it, I also added a "Defining the type of your initialState" chapter, as that was something of importance we had not mentioned before.

@markerikson
Copy link
Collaborator

Awright, let's get this in.

@markerikson markerikson merged commit 0f4cdf2 into reduxjs:master Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants