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.

Sign up for GitHub

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): initialize custom elements even when there is no styles #4296

Merged
merged 4 commits into from
Apr 25, 2023
+29 −9
Merged

fix(runtime): initialize custom elements even when there is no styles #4296

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2bb7f25
remove top-level check gating init logic based on build flags
tanner-reits Apr 17, 2023
2386843
add small test to check initialized flag
tanner-reits Apr 21, 2023
25e98fe
prettier because I don't have it auto-format spec files 🤦‍♂️
tanner-reits Apr 21, 2023
353f1cf
Merge branch 'main' into treits/fix/custom-elements-watchers
tanner-reits Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions src/runtime/initialize-component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,11 @@ export const initializeComponent = async (
Cstr?: any
) => {
// initializeComponent
if (
(BUILD.lazyLoad || BUILD.hydrateServerSide || BUILD.style) &&
(hostRef.$flags$ & HOST_FLAGS.hasInitializedComponent) === 0
) {
if (BUILD.lazyLoad || BUILD.hydrateClientSide) {
// we haven't initialized this element yet
hostRef.$flags$ |= HOST_FLAGS.hasInitializedComponent;
if ((hostRef.$flags$ & HOST_FLAGS.hasInitializedComponent) === 0) {
// Let the runtime know that the component has been initialized
hostRef.$flags$ |= HOST_FLAGS.hasInitializedComponent;

if (BUILD.lazyLoad || BUILD.hydrateClientSide) {
Copy link
Contributor

Choose a reason for hiding this comment

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

At first I thought this was a split/shift of the if statement, but looking a little closer, this appears to be subtly different:

-    if (BUILD.lazyLoad || BUILD.hydrateServerSide || BUILD.style) && ...
+    if (BUILD.lazyLoad || BUILD.hydrateClientSide) {

Can you help me understand the reasoning behind the change from hydrateServerSide -> hydrateClientSide?

Copy link
Member Author

Choose a reason for hiding this comment

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

So there are two if checks there actually. The top-most check originally had the check containing BUILD.hydrateServerSide, but I removed the first part of that check because:

  1. The condition of BUILD.style was preventing that block of code to run for dist-custom-elements builds when the component did not have an associated stylesheet. But, if we just removed that portion of the check, then it would never execute for dist-custom-elements builds
  2. All components need to go through initialization anyway (AFAIK), so we should only really prevent that logic from executing if it has already been initialized

The second if statement is nested directly in that first block. I did not change that conditional or it's code block at all except for pulling out the line that sets the hasInitializedComponent flag on the hostRef since that exact code happened in both the if and else blocks.

Let me know if that doesn't make sense or you think my reasoning is wrong @rwaskiewicz

Copy link
Contributor

Choose a reason for hiding this comment

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

So if I'm understanding correctly, the change is basically to make the outer check only whether the component has been previously initialized (hostRef.$flags$ & HOST_FLAGS.hasInitializedComponent) because the code within the block for that if is stuff we always want to run, regardless of whether this is a lazy load, hydrate build, etc.

Then within that block we have two different code paths, one for BUILD.lazyLoad || BUILD.hydrateClientSide and one for everything else (which is if I understand correctly where we'll go when dealing with a dist-custom-elements build).

The overall logic I think makes sense to me, something like 'did we initialize? if not, let's initialize. then, what kind of component are we dealing with?' and then we do what's specific to different ways to initialize a component.

Is that an accurate summary? If so I think this looks good to me. I also tried out running this change in the reproduction and confirmed that it works correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! That's exactly it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah OK - thank you both!

// lazy loaded components
// request the component's implementation to be
// wired up with the host element
Expand Down Expand Up @@ -81,7 +78,7 @@ export const initializeComponent = async (
} else {
// sync constructor component
Cstr = elm.constructor as any;
hostRef.$flags$ |= HOST_FLAGS.hasInitializedComponent;

// wait for the CustomElementRegistry to mark the component as ready before setting `isWatchReady`. Otherwise,
// watchers may fire prematurely if `customElements.get()`/`customElements.whenDefined()` resolves _before_
// Stencil has completed instantiating the component.
Expand Down
22 changes: 22 additions & 0 deletions src/runtime/test/initialize-component.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { getHostRef } from '@platform';
import { Component } from '@stencil/core';
import { newSpecPage } from '@stencil/core/testing';

import { HOST_FLAGS } from '../../utils';

describe('initialize component', () => {
@Component({
tag: 'cmp-a',
})
class CmpA {}

it('should mark the component as initialized', async () => {
const page = await newSpecPage({
components: [CmpA],
html: `<cmp-a><cmp-a>`,
});

const hostFlags = getHostRef(page.root).$flags$;
expect(hostFlags & HOST_FLAGS.hasInitializedComponent).toBe(HOST_FLAGS.hasInitializedComponent);
});
});
3 changes: 2 additions & 1 deletion src/runtime/test/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
"baseUrl": ".",
"paths": {
"@stencil/core": ["../../index.ts"],
"@stencil/core/testing": ["../../testing/index.ts"]
"@stencil/core/testing": ["../../testing/index.ts"],
"@platform": ["../../client/index.ts"]
}
}
}