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(dist-custom-elements): stop duplicate @stencil/core #6109

Conversation

johnjenkins
Copy link
Contributor

@johnjenkins johnjenkins commented Jan 21, 2025

What is the current behavior?

Setting the externalRuntime: false on a dist-custom-elements output, whilst at the same time including a 3rd party component / lib that imports from @stencil/core, causes the whole stencil runtime to be included twice:

  • once as custom runtime (dictated by flags set via features detected on components at build-time).
  • second as the whole runtime (dictated by the 3rd party lib's import)

This duplication in-turn, breaks certain lifecycle events as different internals call different (duplicate) @stencil/core functions.

GitHub Issue Number:

What is the new behavior?

My naive solution is to make sure the same @stencil/core rollup id is included for both 3rd party libs and the direct component imports - the full @stencil/core build - if it's the dist-custom-elements output.

This stops the duplication.

This doesn't have any particularly noticeable impact on bundle size.

Fixes: #6040
Fixes: #4135

Documentation

Does this introduce a breaking change?

  • Yes
  • No

Testing

  • TBD ... not really sure how to

Other information

ATM I have opted for using the same 'custom' stencil core for both src and node_modules but I may have to rethink.

@rjborba
Copy link

rjborba commented Jan 22, 2025

@johnjenkins thank you so much for working on it.
What is needed to move it from Draft to read to be merged? I may be able to help.

@johnjenkins
Copy link
Contributor Author

thanks @rjborba - I just wanted to discuss this with Christian before going forward.
This approach might have some implications, so just need to tweak / test a few things. Shouldn't take long

@johnjenkins johnjenkins marked this pull request as ready for review January 22, 2025 22:51
@johnjenkins johnjenkins requested a review from a team as a code owner January 22, 2025 22:51
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.

LGTM 👍

@christian-bromann
Copy link
Member

@johnjenkins I think we are missing to propagate the lazy variable in some places 🤔

@christian-bromann christian-bromann merged commit dc4c88f into ionic-team:main Jan 24, 2025
70 of 71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants