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

RTK Query: resolve type portability issues #3678

Merged
merged 3 commits into from
Aug 26, 2023

Conversation

markerikson
Copy link
Collaborator

Copy of #3569 by @eric-crowell , rebased against v2.0-integration

@codesandbox
Copy link

codesandbox bot commented Aug 26, 2023

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@netlify
Copy link

netlify bot commented Aug 26, 2023

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

Name Link
🔨 Latest commit 4647d98
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/64ea5e0b8b538b00087c487f
😎 Deploy Preview https://deploy-preview-3678--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codesandbox-ci
Copy link

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 3a323b6:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
@examples-action-listener/counter Configuration
rtk-esm-cra Configuration

@netlify
Copy link

netlify bot commented Aug 26, 2023

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

Name Link
🔨 Latest commit 3a323b6
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/64ea5f29bbc3ab0008258337
😎 Deploy Preview https://deploy-preview-3678--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@markerikson markerikson merged commit b1f4a17 into v2.0-integration Aug 26, 2023
36 checks passed
@tdurnford
Copy link

@markerikson Will this fix be applied to v1.9 as well? If so, when will the next version be released?

@markerikson
Copy link
Collaborator Author

@tdurnford : no, these changes are just against v2.0 .

There's too many differences in build setup and configuration to try to backport them to 1.9.x at this point.

(Somewhat related: we'd reallllly like people to try out the 2.0 betas and give us feedback on how they work.... :) )

@tdurnford
Copy link

tdurnford commented Sep 15, 2023

@markerikson I'm happy to test betas, but I can't take those to production 😢

Is there a GA target date for 2.0 and do you have migration docs yet?

@markerikson
Copy link
Collaborator Author

markerikson commented Sep 15, 2023

@tdurnford No specific target date, unfortunately, and it's probably going to be several months.

There's a not-well-specified but "large" amount of stuff that needs to be done across all our libraries so that we can publish them simultaneously.

The closest to a known list of stuff we know ought to be done just for RTK itself is in:

I did also try to do a skim through the other repos to get a sense of what things look immediately obvious to do, but don't have that list on hand atm.

On top of that, there's still a number of open questions:

  • What breaking changes should we make to the RTK Query API?
  • What future RTKQ features can we defer for after 2.0?
  • What changes do we maybe need to do now in order to make those later features possible?
  • What about all the codegen stuff?
  • What about proof-of-concept features like proxy-based tracking for selectors in Reselect and React-Redux?
  • We've added several new features like combineSlices and createSlice.selectors, but no one has really tried them out. We think they probably work and have tests... but is the API design right? Do those actually solve real problems? Do they solve problems the right way? Are there use cases we should be considering beyond those? Other API design changes?

You also bring up a good point that we need to consider the rest of the docs work as well.

The next overall steps I need to do are putting on my PM hat and trying to nail down what the actual remaining scope looks like.

I don't want this effort to keep dragging on, and I don't want us to get bogged down chasing one potential feature after another. But given that this is the first major version bump we've ever done for RTK, I also don't know when we might do another one. So, I would rather err on the side of making sure we've covered as many bases as possible.

The other complication, of course, is that we're doing this in our spare time. (That is not a complaint - that's an observation). Lenz hasn't had much time to work on Redux since joining Apollo. Ben has done a lot of work building features, but not much on infra. I'm juggling Replay, Redux, other tasks, and trying to have something resembling a life :) (golf, friends, family, down time.)

I do want to focus on RTK work over the next few months, but it's all in my free time. With golf season ending, I may be able to put a bit more focus into it.

We don't have an actual migration guide written up. The closest to that would be reviewing each of the release notes entries for listed breaking changes - I've tried to be pretty thorough in that regard.

In theory, the biggest known breaking changes would be things like removing the object arguments for createSlice.extraReducers and a bunch of TS types tweaks to change AnyAction to UnknownAction.

So, my general suggestion is to literally try bumping your deps, see what compiles and fails, fix TS compilation issues, and see how your tests shake out.

@tdurnford
Copy link

@markerikson Thank you for the update. I understand that is it important to be thorough when you do a major bump - you don't get very many opportunities and you don't want to be haunted by your choices.

I updated one of my side projects to use the new beta version. Overall, the migration was very smooth. I'll be sure to log any bugs or feature requests I come along.

In the interim, I understand why you don't want to take the fix into 1.9.x, but do you have a potential mitigation? Our project is heavily dependent on @reduxjs/toolkit, and the portability issue is causing major problems for us.

Thank's again and I hope you have plenty time away from your keyboard to enjoy your family and golf while the weather is still nice - very important.

@markerikson
Copy link
Collaborator Author

@tdurnford : yeah, lemme clarify my stance here.

I've got enough on my plate that I don't have time to try to backport this right now. Also, there was a lot of fiddly changes around types paths and references to /dist/ that we had to change here in v2.0-integration. Given that, I don't want to spend time myself backporting these.

However, if you or someone else can take the time to try backporting the changes to 1.9.x and see if they fix things there, sure, I'll happily accept a PR that improves things.

@tdurnford
Copy link

@ericanderson I can spend some time looking into it. Can you point me to some of the fiddly PRs that were necessary so I can get a general sense of the requirements?

@markerikson
Copy link
Collaborator Author

Yeah, some combination of #3605 , #3592 , and #3672 .

@tdurnford
Copy link

Hey, @markerikson, I have opened a draft PR #3728. Still verifying the fix actually works and solves our issue before publishing. Would you mind taking a look. Happy to make updates if needed.

@tdurnford
Copy link

@markerikson I wanted to check in to see if you've had a chance to take a look at my PR

@markerikson
Copy link
Collaborator Author

@tdurnford : nope, sorry. Haven't had time to do any Redux maintenance work the last couple weeks. I'll try to look at it when I have time, but got too many other things going on right now.

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.

3 participants