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 @babel/core dependencies to react/preact integrations #3928

Merged
merged 2 commits into from
Jul 14, 2022

Conversation

matthewp
Copy link
Contributor

Changes

  • @astrojs/react and @astrojs/preact depend on @babel/plugin-transform-react-jsx which as @babel/core as a peerDependencies.
  • When you install either integration you get what looks like an error message (it's actually a warning).

Screen Shot 2022-07-14 at 2 15 51 PM

  • This adds @babel/core as a dependency, fixing the issue.

Testing

N/A

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Jul 14, 2022

🦋 Changeset detected

Latest commit: e06f7d5

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

This PR includes changesets to release 5 packages
Name Type
@astrojs/preact Patch
@astrojs/react Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/ts-resolution Patch

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: preact Related to Preact (scope) pkg: react Related to React (scope) pkg: integration Related to any renderer integration (scope) labels Jul 14, 2022
@matthewp matthewp self-assigned this Jul 14, 2022
@@ -31,6 +31,7 @@
"dev": "astro-scripts dev \"src/**/*.ts\""
},
"dependencies": {
"@babel/core": ">=7.0.0-0 <8.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

@babel/core is only used inside of astro, so I think adding this to a peerDependencies" instead of dependencies` would also silence the error but not result in adding this as an explicit dependency (since pnpm/yarn support bubbling up peer dependencies this way).

Worth confirming, but I think that's the better move here if it works!

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, small hiccup with this approach, astro add automatically installs any peerDependencies from these packages into your Astro project. :(

Not sure the best way to handle this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried it and didn't work unfortunately. I think by making it a peerDependency you are making the user install it.

└─┬ @astrojs/react
  ├── ✕ missing peer @babel/core@">=7.0.0-0 <8.0.0"
  └─┬ @babel/plugin-transform-react-jsx
    ├── ✕ missing peer @babel/core@^7.0.0-0
    └─┬ @babel/plugin-syntax-jsx
      └── ✕ missing peer @babel/core@^7.0.0-0

Copy link
Member

Choose a reason for hiding this comment

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

hmm, that's surprising since Astro has it as a dependency. I actually thought that having it as a dep in astro solved this, so maybe it's new behavior from pnpm?

I guess I'm not against leaving it in deps then!

@matthewp matthewp merged commit d6dfef0 into main Jul 14, 2022
@matthewp matthewp deleted the react-peer-deps branch July 14, 2022 18:40
@astrobot-houston astrobot-houston mentioned this pull request Jul 14, 2022
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: preact Related to Preact (scope) pkg: react Related to React (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants