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(react): add react, react-dom and lit/react to vite externals #594

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

dwirz
Copy link
Contributor

@dwirz dwirz commented Jan 10, 2025

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally for affected output targets
  • Tests (npm test) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Dependency changes were necessary because it fixes a problem with react and next.

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Currently, the generated React components cannot be used with Next.js 15 and React 19, although they do work with Next.js 15 and React 18. The issue lies in the Vite build of @stencil/react-output-target, which bundles react and other dependencies into the distribution output.

Issue URL: ionic-team/stencil#6059

What is the new behavior?

With this change, react, react-dom, and @lit/react are no longer bundled into the output code of the @stencil/react-output-target.

Does this introduce a breaking change?

  • Yes
  • No

Other information

None.

@dwirz dwirz requested review from a team as code owners January 10, 2025 08:33
@dwirz dwirz changed the title Fix/do not bundle deps fix(react): add react, react-dom and lit/react to vite externals Jan 10, 2025
@dwirz
Copy link
Contributor Author

dwirz commented Jan 10, 2025

@christian-bromann The actual change is just this line: https://github.com/ionic-team/stencil-ds-output-targets/pull/594/files#diff-28dac5df05657e5e97b4a2230a26f8314d5920e563809a5422407526e813291aR17. However, I also added another Next.js example project using React 19 to demonstrate that it works. Additionally, I had to move and update some dependencies—I hope that’s okay. Let me know what you think! 🙏

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Changes look good to me , good catch 👍

@christian-bromann
Copy link
Member

@dwirz mind rebasing the PR on top of main?

@dwirz dwirz force-pushed the fix/do-not-bundle-deps branch from 26dad49 to d65f69f Compare January 10, 2025 21:02
@christian-bromann christian-bromann merged commit 6a6a7b5 into ionic-team:main Jan 10, 2025
3 checks passed
@dwirz
Copy link
Contributor Author

dwirz commented Jan 12, 2025

Big thanks @christian-bromann for merging the PR and pushing out a release! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants