-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat!: react 19 support #2368
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
base: next
Are you sure you want to change the base?
feat!: react 19 support #2368
Conversation
- fix: no initial value provided for useRef
This commit update the context creation to match react 19 types while ensure legacy compatibility.
As this release target React 19, the following commit accept any React version in the 19 range.
🦋 Changeset detectedLatest commit: e75557b The changes in this PR will be included in the next version bump. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hey! Yeah the linting/formatting parts of my PR caused me a bit of a headache too... I did run the formatting script before committing anything, which I would have thought would solve the problem! I guess not! I would actually say that the underlying problem may be either my individual setup, or the global setup for linting/formatting in the package as a whole. (From personal experience I know how difficult it is to manage it all properly in a massive monorepo). Do you think it'd be easier to wait for my PR to be merged - then applying the formatting fixes? (I'd actually be interested to see if you got any different results from running the Realistically if the issue is stemming from a formatting mismatch from either my machine or the monorepo as a whole - Fixing that may help to resolve the problem? Obviously i'm happy to pull out the core parts of my PR into this PR if that's truly necessary |
@kierancap thanks for you answer. After looking back at your PR, it seems that the formatting is a smaller problem since your latest commits. There are way fewer space changes. So this is not a problem anymore. I rebased your work on my PR so here is a change log. So you have the choice to work on this one or yours. Fixed:Use of "any"We were losing a lot of benefit from TypeScript static typing. This was caused by the partial changes to the peer and dev dependencies. There are a lot of packages to update including React, React-dom, drei, and three-fiber which prevents the need to "any" all the "Tag" related types. Using null over undefinedSome "useRef" are initialized with "null" while they were initialized with implicit undefined. before: which is different from To fix:Potential breaking change on the context.I rebased the work on jest setup and SprintContext so all the tests pass now. But there is a catch. Those changes were made: (Note : I use The current version exports a context object with the following signature : But now it splits the exported context in two parts: Given that the context provider was exported at the package level, it will break any project that uses Ex: If my project uses the old version, switching to this new version will break this code: import { SpringContet } from 'react-spring';
export myComp = function({children, ...props}) {
return <SpringContext {...props}>
{children}
</SpringContext>
} So, either we bump to a major version due to the breaking change, or we stay at patch/minor level and find a way to keep the same package interface. |
Hey! So... AnyThe any was due to polymorphic components (if i'm remembering right - really annoying). If you were able to tidy those up that's great! RefsCompletely agreed on the approach to remove ContextI'm nearly certain that with the newest types derived from React, that merging a Provider AND Context initiator into the same object will be nigh on impossible. That's why that old setup was so un-idiomatic and seemed to chip away at the underlying context logic to allow for it to happen. I would definitely consider a Major bump here - Josh also agreed on the context change in my initial PR. The Provider paradigm for Context is quite popular these days, and is way simpler on the DX to use. Obviously it does give us a breaking change, but for overall maintainability I think that's a valued change |
@kierancap about the breaking change: indeed, I found the message where @joshuaellis agrees. How do you want to proceed? Either you cherry pick from mine and I close this one or we continue on this one. It remains:
|
BREAKING CHANGE: updating to React 19 brings a breaking change to the SpringContext API. To add the Spring context provider, you must explicitly use SpringContextProvider. before: ```tsx import { SpringContext } from 'react-spring' ... <SpringContext ...> ... ``` now: ```tsx import { SpringContextProvider } from 'react-spring' ... <SpringContextProvider ...> ... ```
Assuming the breaking change, the minimum requirements is now React 19
|
added a breaking change section in doc |
fix types error in demo due to mutliple react version:
|
targets/native/tsup.config.ts
Outdated
@@ -6,6 +6,7 @@ export default defineConfig(opts => { | |||
{ | |||
entry: 'src/index.ts', | |||
name: 'react-spring_native', | |||
buildFilter: ['development', 'production.min'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a filter to build only what needed (use case: React Native just needs CJS format)
options: Options | ||
): Options[] => { | ||
const artifactOptions: Options[] = buildTargets.map( | ||
const artifactOptions: Options[] = buildTargets | ||
.filter(target => !buildFilter || buildFilter.includes(target.name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshuaellis about removing the ESM build for React Native.
I use the filter here but I have an issue. I am new to turbo, so I surely miss something.
Here is a view of my logs during the build. In pink, the log for the native target show that the build are well filtered.
But the ESM flavor files still lands in the dist folder.
Have you any idea why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry why do we want to remove ESM from react-native?
Is that what other libraries do? That feels weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshuaellis I explain this here #2368 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved here @joshuaellis
The esOptions being present, it seems to automatically trigger the ES build regardless of the module resolution context. So I put a condition inside.
Test still passes, no regression detected.
gentle ping @joshuaellis |
@@ -14,14 +14,14 @@ | |||
], | |||
"compilerOptions": { | |||
"target": "ES2020", | |||
"module": "ESNext", | |||
"module": "NodeNext", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we need to get rid of ESM in the native target, we need to handle both ESM and CJS output format. Which is supported by NodeNext.
Otherwise, @react-sping/native definition will not be found at code time and crashes on tests and lint.
@@ -48,7 +48,7 @@ | |||
}, | |||
"peerDependencies": { | |||
"@react-three/fiber": ">=6.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to bump here ?
f3e28e5
to
e75557b
Compare
|
Tested the |
This first is up to date from the below discussion so you don't waste time to read all the thread**
--> CURRENT RELEASE: 10.0.0-beta.0 <--
TLDR;
Todo
Why
Support for React 19.
This PR is the last attempt in date to support React 19. It follows the tremendous effort which have been poured into #2363—as a great example of community effort. Kudo kierancap for the work on the Context.
Notable changes
BREAKING CHANGE:
OTHER CHANGES: