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

Fix #212 #214 #215

Merged
merged 4 commits into from
Oct 16, 2019
Merged

Fix #212 #214 #215

merged 4 commits into from
Oct 16, 2019

Conversation

phryneas
Copy link
Member

This should fix a bug with TS3.6 found in #212 and the bug reported in #214.

This should be working fine, but I'm currently preparing a conference talk, so I don't have the time to test this super-excessively and the bug fix for #214 is something I'm not feeling 100% sure of if I did everything right.
@dragomirtitian, could I ask you to take a look that I didn't do anything incredibly stupid there?

@netlify
Copy link

netlify bot commented Oct 15, 2019

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

Built with commit 5db52ae

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

@@ -13,8 +13,8 @@ export type IsUnknown<T, True, False = never> = unknown extends T
: False

export type IsEmptyObj<T, True, False = never> = T extends any
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 I'm most concerned with. I believe this should be good to find really empty objects (the one before also matched objects with only optional properties), but I'm not 100% sure I'm not missing something.

Choose a reason for hiding this comment

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

Your intuition is right! As far as I know, that's the most reliable way to match an empty object. {} is very tricky, since it doesn't really mean empty object. 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks a lot, that gives me some assurance :)

Choose a reason for hiding this comment

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

The empty object detection looks good to me too. keyof T extends never is the most reliable way to ensure that the type has no keys.

@markerikson
Copy link
Collaborator

Great, thanks!

Out of curiosity for the still mostly-TS-clueless: any chance you could walk through how you went about debugging this problem?

@phryneas
Copy link
Member Author

phryneas commented Oct 15, 2019

Hm, that failing build was to be expected (although it funnily doesn't fail on my local system, but I am switching between TS versions like crazy...). @markerikson, would you be okay if I added an any-cast in that failing line?

@phryneas
Copy link
Member Author

phryneas commented Oct 15, 2019

Great, thanks!

Out of curiosity for the still mostly-TS-clueless: any chance you could walk through how you went about debugging this problem?

I went to the PayloadActionCreator type and replaced each possible result with a string, to see which path it goes. It went to ActionCreatorWithPayload, so I did the same thing there, only to see that it took a wrong branch in IsUnknownOrNonInferrable - and so on.

I'm sure there are more elegant solutions to debug that, but that dumb way is working pretty well 😄

@markerikson
Copy link
Collaborator

Ah, ye olde "brute force logging" approach :)

Yeah, I'm okay with whatever works to actually resolve the problem.

@phryneas
Copy link
Member Author

Argh, failing due to prettier issues. I really have to stop procastinating, I'll fix that up tomorrow. Alternatively, @markerikson could you please just push a prettier fix?

@markerikson
Copy link
Collaborator

markerikson commented Oct 15, 2019

At work atm, but yeah, will do so later.

Also, Y U NO PRETTIER AUTOFORMAT ON SAVE?!?!?

@phryneas
Copy link
Member Author

Also, Y U NO PRETTIER AUTOFORMAT ON SAVE?!?!?

Not my work laptop, and vscode defaults to some other autoformat behaviour than prettier. But it does something. So I always forget to install the plugin, because it looks right. Fixed that on two machines and today, wrote the fix on my third machine :D

@markerikson
Copy link
Collaborator

Thanks for the fast solution!

@markerikson markerikson merged commit c56b76b into reduxjs:master Oct 16, 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.

None yet

4 participants