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 hover preview of generated selectors. #633

Merged
merged 5 commits into from
Nov 1, 2023

Conversation

aryaemami59
Copy link
Contributor

@aryaemami59 aryaemami59 commented Oct 31, 2023

This PR aims to flatten and expand the types for generated selectors in order to improve their visual display inside hover previews.

This is what it looked like before the changes were made:

Inside typescript_test/test.ts on line 1269:

before

And this is what it looks like after changes were made:

Same selector inside typescript_test/test.ts now on line 1319:

after

Other minor changes include:

  • Remove left over types related to ts46-MergeParameters.
  • Reorganized types.ts by category.
  • Fix createStructuredSelector type inference.
  • Remove second overload of StructuredSelectorCreator, since the function itself has no behavior indicating it would need multiple function signatures.
  • Create TypedStructuredSelectorCreator which is used to effectively replace StructuredSelectorCreator second function signature (WIP).
  • Add more JSDocs.
  • Add testUtils.ts which contains some utilities that will aid in unit testing.
  • Add more type tests.
  • Add more unit tests.
  • Fix infinite type instantiation issue that was caused inside of deepNesting function in typescript_test\test.ts file.
  • Add public and internal tags to existing JSDocs.
  • Remove non-existent rules from .eslintrc.
  • Add @typescript-eslint/consistent-type-imports rule to help with auto-fixing type imports.

- Remove left over types related to `ts46-MergeParameters`
- Reorganized `types.ts` by category.
- Fix `createStructuredSelector` type inference.
- Remove second overload of `StructuredSelectorCreator`.
- Create `TypedStructuredSelectorCreator` which is used to effectively replace `StructuredSelectorCreator` second function signature (WIP).
- Add more JSDocs.
- Add `testUtils.ts`.
- Add more type tests.
- Add more unit tests.
- Fix infinite type instantiation issue that was caused inside of `deepNesting` function in `typescript_test\test.ts` file.
- Add public and internal tags.
- Remove non-existent rules from `.eslintrc`.
- Add `@typescript-eslint/consistent-type-imports` rule to help with auto-fixing type imports.
Copy link

codesandbox-ci bot commented Oct 31, 2023

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 2614993:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

@aryaemami59 aryaemami59 marked this pull request as ready for review October 31, 2023 19:23
@aryaemami59
Copy link
Contributor Author

@markerikson This also fixes the TS recursion problem so now we can do this:

deep-nesting

and this:

param-list

@markerikson
Copy link
Contributor

ho. ly. smoke.

that is amazing!

lemme actually read through the changes, but wow, I'm seriously impressed at the apparent results!

@aryaemami59
Copy link
Contributor Author

ho. ly. smoke.

that is amazing!

lemme actually read through the changes, but wow, I'm seriously impressed at the apparent results!

Thank you, it feels good to hear that. I don't know if I made it clear enough in the JSDocs and commit messages, but the reason the infnite instantiation bug was happening is because, TypeScript was passing generic parameters into other types that then took those generic parameters and tried to instantiate a type by doing some calculation. The problem was that it was trying to do all of the instantiation using recursion all at the same time which caused the bug in the first place. What I did was that I tried to interrupt the recursion process by expanding and flattening the types (in this case OutputSelector) right before the recursion process begins. So TypeScript now instantiates the type and caches it before recursion so it doesn't have to do all of the work at the same time. I might be completely wrong, but to me it seems like that is what is happening.

@markerikson
Copy link
Contributor

Okay, skimmed through. Obviously that's a pretty big diff, enough so that we're definitely into "LGTM 🤷‍♂️ !" territory :) But the changes that I saw seemed to make sense, and that hover output is exactly what I was hoping to see. Honestly the only tiny half-nitpick I had was that the JSDoc example for createStructuredSelector probably doesn't need to have the actual items arrays filled out, just to save some visual space.

But no reason to hold up this PR for that :)

Meanwhile, totally unrelated to this repo: we're having some active debates about the design of the new createSlice.selectors` field. Latest set of questions is in reduxjs/redux-toolkit#3387 (comment) . Would love to have you offer some feedback and help us figure that out so we can finish RTK 2.0!

@markerikson markerikson merged commit 9966bf2 into reduxjs:master Nov 1, 2023
15 checks passed
@aryaemami59
Copy link
Contributor Author

Okay, skimmed through. Obviously that's a pretty big diff, enough so that we're definitely into "LGTM 🤷‍♂️ !" territory :) But the changes that I saw seemed to make sense, and that hover output is exactly what I was hoping to see. Honestly the only tiny half-nitpick I had was that the JSDoc example for createStructuredSelector probably doesn't need to have the actual items arrays filled out, just to save some visual space.

But no reason to hold up this PR for that :)

Meanwhile, totally unrelated to this repo: we're having some active debates about the design of the new createSlice.selectors` field. Latest set of questions is in reduxjs/redux-toolkit#3387 (comment) . Would love to have you offer some feedback and help us figure that out so we can finish RTK 2.0!

You're right about the JSDocs, my apologies, I will fix that right away. About the createSlice.selectors, I was actually working with them today, I will join the discussion shortly. And please let me know if there anything else I can help with.

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