-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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: adapter recognizes react 19 react.transitional.element #12685
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 9861f42 The changes in this PR will be included in the next version bump. 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 |
This is great, thanks! Could you either update the package fixture to use React 19, or create a copy that is updated so there's still a test for React 18? |
Can do. Something changed in React 19 that caused the tests to fail, either that or maybe there's a mismatch in React versions. (not entirely sure yet how the fixtures resolve react dependency) I will probably have a look over the weekend |
… fix/react19-element-rename
…ment and 1 using NODE_ENV production
@ascorbic The tests are working again! It turns out the issue was caused by the fixtures importing the development build of React while rendering with the production build. This behavior rendered fine prior to React 19, but not anymore. Also added a solid component since the error only occurred when using two frameworks that share the same file extension. One caveat: if windows is the targeted platform, |
to be honest it would be ideal if it was easier to test for multiple versions of a library but i think that's beyond the scope now |
You can, actually, and it's easy. In the fixtures, instead of using |
But this requires a copy of the fixtures no? I was looking for a way to do it on test level |
Yeah, in this case, you should create two fixtures. And you'll have one test file where we load both fixtures (a test case for fixture). That's the expected way to go, we do that all the time :) |
Yeah unfortunately this doesn't really work because I was thinking of using npm aliases but then you need a different import statement in fixtures as well. |
I believe there's a misunderstanding. I'll see if I can help when I have time |
I've done a similar change in #12948 before noticing this. Sorry about that. I couldn't quite figure out how to fix the test though, but it seems like you did, so I think it's still valuable if we can get that merged in. #12686 (comment) |
Don't worry, no hard feelings. I was able to "fix" the tests by copying existing react 18 tests and using separate fixtures for react 18 and react 19. It's not ideal. I don't mind implementing an alternative solution. If you or @ematipico could have a quick look, that'd be great. Could hop on discord too if you want me to show what's the issue. |
I think it's ok to drop the react 18 tests if it's making things harder, or if it helps simplify the setup. We also support react 17 but we don't really test it anymore. Should probably release a new major with only the latest react major at some point. |
Changes
Closes #12684
React 19 renamed react.element to react.transitional.element. (facebook/react#2881)
@astro/react is updated to take this the new name into account, in addition to supporting the previous one.
Testing
Ran automated tests on React 18.3.1. They still pass. Upgraded to React 19.0.0, then they all fail due to another issue. Using the build from this pr in the stackblitz from #12684 causes it to render fine.
Docs
No changes in user behavior.