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

Add support for React 18 in @astrojs/react #2947

Merged
merged 12 commits into from
Mar 31, 2022
Merged

Add support for React 18 in @astrojs/react #2947

merged 12 commits into from
Mar 31, 2022

Conversation

FredKSchott
Copy link
Member

Changes

  • Adds support for v18 to @astrojs/react
  • React v17 remains the same
  • Monorepo stays on v17 to confirm that this doesn't break
  • Follow up PR to update Monorepo to v18

Testing

  • Tested manually
  • I believe this is impossible to test in our monorepo, due to how we use peer deps. In the monorepo, the peer dep is hard-coded do whatever the actual dep is of the integration. Because its a symlink, it can't follow the import the way it would in a normal project. This shouldn't impact all monorepos, just ours where the integration source code itself is literally the thing being mono-repo'd.

Docs

  • N/A

@changeset-bot
Copy link

changeset-bot bot commented Mar 31, 2022

🦋 Changeset detected

Latest commit: bf8497e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/react Minor

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 the pkg: astro Related to the core `astro` package (scope) label Mar 31, 2022
@@ -17,7 +18,7 @@ function getRenderer() {
{},
{
runtime: 'automatic',
importSource: '@astrojs/react',
importSource: ReactVersion.startsWith('18.') ? 'react' : '@astrojs/react',
Copy link
Member

@delucis delucis Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this all LGTM, but is this line intentional? Why does this toggle between importing the integration and importing React itself here?

(The tests are passing but I don't think the first half of this ternary expression is ever hit at the moment because tests are still using React 17.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, mind explaining what this is doing @FredKSchott ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, the docs for the exist but they're in jsx-runtime.js. This actually tripped me up, so I'll add this line of docs here:

// This option tells the JSX transform how to construct the "*/jsx-runtime" import.
// In React v17, we had to shim this due to an export map issue in React.
// In React v18, this issue was fixed and we can import "react/jsx-runtime" directly.
// See `./jsx-runtime.js` for more details.

@matthewp matthewp merged commit 57f48b2 into main Mar 31, 2022
@matthewp matthewp deleted the delucis/react-18 branch March 31, 2022 16:51
@github-actions github-actions bot mentioned this pull request Mar 31, 2022
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* First pass at supporting React 18 in @astrojs/react

* Try marking React 18’s `react-dom/client` as external

* Try a different approach to importing different React versions

* Allow resolving JSON modules

* Revert "Allow resolving JSON modules"

This reverts commit 5279b72.

* Try the separate client entrypoint approach from withastro#2946

* Clean up diff

* Trying to see something

* Just keep swimming… 🐠

* update to support react 18

* add changeset

* add docs

Co-authored-by: delucis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants