fix(custom-elements): hydrate on client side

[andrew9994]

What is the current behavior?

Components hydrated on the server are hydrated wrongly on the client using the dist-custom-elements output target.

GitHub Issue Number: #3319

What is the new behavior?

By setting build.hydrateClientSide = true in custom-elements-build-conditionals.ts and by disabling lazy loading, the components are rendered correctly.

Documentation

Reproduction repo:

https://github.com/andrew9994/stencil-hydrate-dist-custom-elements-bug

Does this introduce a breaking change?

• Yes
• No

Testing

To completely unit test these modifications a change in the spec-page.ts would be needed and I was not sure that I should change that. Instead I added unit tests to check for the default values, and added missing unit tests for thehasHydrateOutputTargets logic in the lazy-build-conditionals.ts

Screenshots from reproduction repo

Before:

After:

[github-actions]

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1139 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files

Path	Error Count
src/dev-server/index.ts	37
src/dev-server/server-process.ts	32
src/compiler/prerender/prerender-main.ts	22
src/testing/puppeteer/puppeteer-element.ts	22
src/runtime/client-hydrate.ts	20
src/screenshot/connector-base.ts	19
src/runtime/vdom/vdom-render.ts	17
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.ts	14
src/compiler/transpile/transpile-module.ts	14
src/sys/node/node-sys.ts	14
src/compiler/prerender/prerender-queue.ts	13
src/compiler/sys/in-memory-fs.ts	13
src/runtime/connected-callback.ts	13
src/runtime/set-value.ts	13
src/compiler/output-targets/output-www.ts	12
src/compiler/transformers/test/parse-vdom.spec.ts	12
src/compiler/transformers/transform-utils.ts	12
src/mock-doc/test/attribute.spec.ts	12

Our most common errors

Typescript Error Code	Count
TS2322	361
TS2345	349
TS18048	201
TS18047	82
TS2722	37
TS2532	24
TS2531	21
TS2454	14
TS2790	11
TS2352	10
TS2769	8
TS2538	8
TS2416	6
TS2493	3
TS18046	2
TS2684	1
TS2430	1

Unused exports report

There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports

File	Line	Identifier
src/runtime/bootstrap-lazy.ts	21	setNonce
src/screenshot/screenshot-fs.ts	18	readScreenshotData
src/testing/testing-utils.ts	198	withSilentWarn
src/utils/index.ts	145	CUSTOM
src/utils/index.ts	269	normalize
src/utils/index.ts	7	escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts	25	BUILD
src/compiler/app-core/app-data.ts	115	Env
src/compiler/app-core/app-data.ts	117	NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts	123	updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts	61	satisfies
src/compiler/types/validate-primary-package-output-target.ts	61	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

[christian-bromann]

@andrew9994 thanks for raising the PR, mind resolving the prettier issues by running npm run prettier?

@ionic-team/stencil I was able to reproduce the bug and verified that the patch resolves the problem. Removing BUILD.hydrateClientSide from the if statement seems like a very deliberate change. I found it being introduced in a massive PR many years ago. I wonder if this might have an impact or side effect on other hydration cases. That said, the unit tests seem to be passing. I don't know enough to make confident decision here but from POV the change looks fine. Curious about your thoughts.

[christian-bromann]

I similar PR was introduced by @sean-perkins some time ago: #3330

[andrew9994]

Thanks for the quick review @christian-bromann! I resolved the code-style issues 👍

[tanner-reits]

@andrew9994 I tried this out in the original reproduction from #3319, but the issue still appears to persist. Did you happen to try your solution out there and see something different?

[andrew9994]

@tanner-reits, good catch, it turns out the fix wasn't working in the original reproduction repository. Apologies for not testing it there. I identified the main issue: the defineCustomElement function was being called while the document was still loading. To address this, I moved the built bundle into the <body> - with this change the fix now works as expected. (I also made additional commits in that repository to simplify the local linking of @stencil/core)

It's important to note that this fix will only work when the document has finished loading and is in the interactive/complete state. Do you see an issue regarding this?

[github-actions]

PR built and packed!

Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8108739095/artifacts/1288698774

If your browser saves files to ~/Downloads you can install it like so:

unzip -d ~/Downloads ~/Downloads/stencil-core-4.12.4-dev.1709282561.59e4ec2.tgz.zip && npm install ~/Downloads/stencil-core-4.12.4-dev.1709282561.59e4ec2.tgz

[andrew9994]

@tanner-reits, @rwaskiewicz could you give an update on this? Can we expect a merge in the foreseeable future? If not, should I look for a fix in the runtime, instead of changing the build-flags?

[christian-bromann]

could you give an update on this? Can we expect a merge in the foreseeable future?

I am currently reviewing this with the team and will post updates as soon as we have them.

[christian-bromann]

@andrew9994 thanks for taking a stab at this. The team has looked into ramifications of this change and has identified two suggestion to reduce risk of introducing any side effects. Let me know what you think of these suggestions.

[andrew9994]

Thanks @christian-bromann and to the team for the review 🙏 I implemented your suggestions

[christian-bromann]

LGTM 👍

I will have someone else from the @ionic-team/stencil team review this.

[rwaskiewicz]

Thanks!