fix(react-form): fix cache components support #1913
Conversation
Attempts to resolve TanStack#1907 Removes `Math.random()` introduced in TanStack#1893 from the hot path allowing the page to be pre-built with a a user provided form id. source: https://nextjs.org/docs/app/getting-started/cache-components#non-deterministic-operations
🦋 Changeset detectedLatest commit: 7546228 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
crutchcorn
left a comment
crutchcorn
left a comment
There was a problem hiding this comment.
I'm unclear on if this is expected behavior, either.
At very least we cannot merge this PR because of the major bump it would trigger due to the changeset file.
Let's have @harry-whorlow and @AlemTuzlak look into this
|
@crutchcorn ah I must have misunderstood the PR instructions. I will update the changeset file :) |
|
S'all good :) Changeset is a tricky tool to get used to |
|
Thank you so much for the pointer. Please let me know if changing it to patch is enough or if the file should be fully removed. It's my first contribution here and I am learning the ropes ;) Also you mentioned that you are unclear if this is expected behavior either. I am not sure what that means... I think the proper fix for this is to use |
I think you're right...
And also right here. What if we detect That shouldn't break the rules of React hook, especially if we do that replacement like so: import {version as reactVersion, useId} from "react";
function useUUID() {
/* UUID code here */
}
const useFormId = Number(reactVersion.split(".")[0]) > 17 ? useUUID : useId;WDYT? Aside about contributing
For the record, even if this PR isn't merged exactly as-is, please don't hesitate to find other tickets and/or reach out to be mentored (by me!) on how to continue contributing. We always enjoy and welcome new contributors. |
|
View your CI Pipeline Execution ↗ for commit 7546228
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 9ec39bb ☁️ Nx Cloud last updated this comment at |
|
@mhornbacher wanna test this in your project before we go live? :) Check the install commands here: #1913 (comment) Or by using: npm i https://pkg.pr.new/@tanstack/react-form@1913And let me know if this fixed the bug. If it did, we'll merge and release ASAP |
|
@crutchcorn yes. Sorry it took a minute, I wanted to isolate other possible causes but I can confirm that this does not fix the issue Testing component (sorry for the ugly code) I will have to dig deeper to find the cause. Are you ok if I use the CI pipeline here to push some experiments to try and isolate it? (do you pay for CI runs/package hosting or is it covered in a GitHub/StackBlitz OSS tier) "use client";
import { useForm } from "@tanstack/react-form";
export function ForgotForm() {
const form = useForm({}); // commenting out this line enables pre-rendering.
return <>{'hi'}</>
}Before and after removing 
|
|
Glad we were able to confirm that this doesn't fix - that's better than pushing and being off-base.
Go for it! We don't pay for hosting, we're covered by OSS. If you need any help debugging, let us know in the Discord and we'll help however we can :) |
|
@crutchcorn I have not found the cause yet but I was actually able to confirm that However it does cause an annoying hydration issue and I went through each execution line in a debugger and am fully lost as to what could possibly be causing this. maybe a set of fresh eyes tomorrow will make it clear :( |
|
Makes sense. I'm game to merge to fix the hydration issues. Need anything from us before we merge for that purpose? EDIT: I realized a problem. We can't import useId from React. We need to do React.useId and import React via |
|
Its all good. I do want to try and help fix the root problem if I can. I am more of an application and infrastructure engineer than a library maintainer so I am still getting used to the concerns of library development. Its a different world and its fun :) For the project I am building it is really a non-issue because we can wrap the pages that need a form in a |
No problem. The original |
|
Yup! And agreed haha. Library code is very different but rewarding |
|
I pushed a I'll update it to the |
|
Just for the future, useMemo is not guaranteed to run only once. |
|
Oh, I did not know that. Thank you! |
|
I got somewhere! I setup a minimal reproduction within the Sorry I am just overly excited |
|
Oh! Interesting! We can't have crypto calls in our codebase because we need to support React Native as well, just FYI. That is to say that when you find it, hunt it down and kill it with prejudice haha |
|
Found it here in https://github.com/TanStack/pacer: https://github.com/TanStack/pacer/blob/6238bcf592edf01bb188ea0b412026692fc24513/packages/pacer/src/async-retryer.ts#L365-L383 It is related to the DevTools so likely not running in React Native at all :) What is strange is that this is in the |
|
Verified that removing the import from Does it make sense to remove it and replace it with a simple |
|
Let's tag in @KevinVandy who maintains Pacer, so we can patch upstream :) |
1 similar comment
|
Let's tag in @KevinVandy who maintains Pacer, so we can patch upstream :) |
|
This was fixed in Pacer a couple weeks ago. I thought @harry-whorlow might be updating soon? Also, TanStack Form should install pacer-lite instead of pacer now for smaller bundle size. |
|
I have an open branch amt, just need to push it... Probably in the morning |
|
@KevinVandy nice! I opened a PR to remove it entirely as its only used in one spot for dev-tools but it looks like you guys are on it! |
|
Pacer is updated in 1876 to pacer lite. What was the issue with changesets? |
Yeah, this breaks react 17, hence why we implemented our own uuid
Crazy, I didn’t realise you could import the react versions. I like this solution. As for the changeset it looks like the perhaps the cli steps were misunderstood, since I can't see anything that would make Changesets bump a major. But yeah a patch version would suffice 😄 |
|
@harry-whorlow neither did I. @crutchcorn is a legend! Do you need me to close this and open a new PR to flush out the major version bump? |
|
@harry-whorlow the issue with patchsets have been updated since my comment :) A bit of back-n-forth yesterday, especially cuz I was on mobile out on the town reviewing. @mhornbacher nope! This looks great to me; merging now after workflows pass. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1913 +/- ##
===========================================
- Coverage 90.35% 54.50% -35.85%
===========================================
Files 38 18 -20
Lines 1752 233 -1519
Branches 444 34 -410
===========================================
- Hits 1583 127 -1456
+ Misses 149 94 -55
+ Partials 20 12 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This is merged into |
|
Thank you guys so very much. Your dedication to this project and its performance for the wider web is above and beyond what anyone could imagine to ask for ❤️ |
4 participants
🎯 Changes
Attempts to resolve #1907
Removes
Math.random()introduced in #1893 from the hot path allowing the page to be pre-built with a a user provided form id.source: https://nextjs.org/docs/app/getting-started/cache-components#non-deterministic-operations
✅ Checklist
pnpm test:pr.🚀 Release Impact