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: adapter recognizes react 19 react.transitional.element #12685

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jeroenpg
Copy link

@jeroenpg jeroenpg commented Dec 8, 2024

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.

Copy link

changeset-bot bot commented Dec 8, 2024

🦋 Changeset detected

Latest commit: 8ca9581

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

@github-actions github-actions bot added pkg: react Related to React (scope) pkg: integration Related to any renderer integration (scope) labels Dec 8, 2024
@ascorbic
Copy link
Contributor

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?

@jeroenpg
Copy link
Author

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

@jeroenpg jeroenpg changed the title fix: adapter recognizes react 19 react.transitional.element [WIP] fix: adapter recognizes react 19 react.transitional.element Dec 14, 2024
@jeroenpg jeroenpg changed the title [WIP] fix: adapter recognizes react 19 react.transitional.element fix: adapter recognizes react 19 react.transitional.element Dec 14, 2024
@jeroenpg
Copy link
Author

@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, react-component-production.test.js is not ran. On windows it seems to ignore overriding NODE_ENV. Perhaps I'm missing something obvious here.

@jeroenpg
Copy link
Author

@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, react-component-production.test.js is not ran. On windows it seems to ignore overriding NODE_ENV. Perhaps I'm missing something obvious here.

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

@ematipico
Copy link
Member

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 workspace:*, install the react version that you need to test.

@jeroenpg
Copy link
Author

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 workspace:*, install the react version that you need to test.

But this requires a copy of the fixtures no? I was looking for a way to do it on test level

@ematipico
Copy link
Member

ematipico commented Dec 17, 2024

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

@jeroenpg
Copy link
Author

jeroenpg commented Dec 17, 2024

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 node:test creates an isolated worker per test file, thus it caches the react import and there's no way to clear the cache yet when using ESM 😔

I was thinking of using npm aliases but then you need a different import statement in fixtures as well.

@ematipico
Copy link
Member

I believe there's a misunderstanding. I'll see if I can help when I have time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) pkg: react Related to React (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React 19 elements not recognized as React Element
3 participants