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(runtime): importing custom elements build in lazy build #3330

Conversation

sean-perkins
Copy link
Contributor

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 and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

You are unable to import and use a custom elements build within a lazy-hydrated bundle Stencil project. When trying to use a Stencil compiled library within another Stencil project, you will receive numerous exceptions & the component will fail to render.

GitHub Issue Number: #2531, #3233

What is the new behavior?

Stencil libraries can be consumed within other Stencil libraries. For example, developers can use @ionic/core/components within their own Stencil libraries.

  • Hydrated web component behavior is skipped when the element being rendered is a custom element
  • Introduces $customElement$ flag on the compiler metadata for the component, so that the runtime can make decisions based on if the component being rendered is a custom element or part of the lazy/hydrated build.

Does this introduce a breaking change?

  • Yes
  • No

Testing

You can either clone this repository: https://github.com/sean-perkins/stencil-ce-project or follow these steps:

  1. Create a new Stencil project npm stencil init
  2. Install another Stencil library as a dependency, i.e. @ionic/core
  3. For Ionic, you will need a global script to initialize required functionality:
// src/global/app.ts
import { initialize } from '@ionic/core/components';

initialize();
  1. Update stencil.config.ts to use the global script:
// stencil.config.ts
{
  ...
  globalScript: './src/global/app.ts'
}
  1. In your new Stencil project, use an Ionic component from the custom elements bundle
// src/components/my-component/my-component.tsx
import { defineCustomElement  } from '@ionic/core/components/ion-button.js';

@Component(...) 
export class MyComponent {
  connectedCallback() {
    defineCustomElement();
  }

  render() {
    return (
       <ion-button>Default</ion-button>
    );
  }
}
  1. Run the project locally and notice the following exception in the console:
Trying to lazily load component <ion-button> with style mode "undefined", but it does not exist.

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'then')
    at initializeComponent
  1. Locally build this PR and install the .tgz
  2. See that the component renders correctly

Other information

These changes were inspired from MarkChrisLevy's fork: https://github.com/CODE-AND-MORE/stencil/commits/codeandmore-2.12.1 and tested in an isolated project. After running into the issue and diagnosing a solution, I found that my approach aligned extremely similar to his.

There is likely additional considerations that need to take place to handle mixing custom elements & the hydrated build, but the current state is completely broken, incrementally improving the experience to initialize the components seems valuable to the community. Also by fixing this issue, I will be able to identify other issues with the build and work to report and resolve those.

@sean-perkins sean-perkins requested a review from a team April 14, 2022 05:12
@sean-perkins
Copy link
Contributor Author

Let me know if the failing CI is a legit thing. Looking at other PRs, looks to be a flaky test/issue with Windows Firefox test automation.

@alicewriteswrongs
Copy link
Contributor

I'm not certain but I think the browserstack CI is just flakiness.

But! I was just able to reproduce the issue, and confirm that this branch fixes it. Reading through the code now!

Copy link
Contributor

@alicewriteswrongs alicewriteswrongs left a comment

Choose a reason for hiding this comment

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

I will defer to @rwaskiewicz on this one, but I think this looks straightforward enough to me!

@rwaskiewicz rwaskiewicz added the Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. label Jun 13, 2022
@orl99
Copy link

orl99 commented Aug 6, 2022

Do we have an update of this PR?

@rwaskiewicz
Copy link
Contributor

Thanks for your patience on this folks!

The team has reviewed this PR, and while it takes the right steps towards fixing the described problem, it doesn't cover all the necessary runtime concerns. As a result, we cannot accept it as-is. We have support for this on our radar, but are going to close this PR while we work through the solution. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants