feat(compiler): Allow disabling the injected hydration stylesheet #2989

splitinfinities · 2021-08-04T16:34:50Z

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
Tests (npm run test.karma.prod) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

GitHub Issue Number: #2245
Internal ticket: STENCIL-23

What is the new behavior?

This change provides a new stencil.config.ts option called invisiblePrehydration, which is defaulted true, and can be set false.

Ionic has an opinion to visually hide components while they are not hydrated by automatically injecting a stylesheet into the head of a document containing a css ruleset setting visibility: hidden on each of compiled components, only until they have a hydrated class.

This lets consumers opt-out of that Ionic opinion, so that they can use their own fallback styles, and more closely resemble the HTML Spec around Shadow DOM, Slots, and Dehydrated Web Components.

Does this introduce a breaking change?

Yes
No

Testing

Check this branch out.
Link the branch with npm link, or build then pack the branch.
Create a new component library with npm init stencil, installing the local copy of stencil with either npm link @stencil/core --force, or npm install <path to stencil repo>/stencil-core-<version>.tgz
Edit the stencil.config.ts file, and add a new line for invisiblePrehydration: false
Run npm start, and when the server starts up, you should not see a style tag automatically injected into the head of the document. You can view source on that html page to see that by default, there is no style tag sent over the wire.
Shut the server down, change the value to true, and you will see the style tag added.

Other information

splitinfinities

I have some changes and notes listed below

src/client/polyfills/css-shim/load-link-styles.ts

src/compiler/app-core/app-data.ts

src/compiler/prerender/prerender-optimize.ts

test/karma/package.json

test/karma/test-app/util.ts

test/karma/test-invisible-prehydration/package.json

rwaskiewicz · 2021-08-18T14:11:29Z

src/compiler/app-core/app-data.ts

@@ -155,6 +155,7 @@ export const updateBuildConditionals = (config: Config, b: BuildConditionals) =>
  b.scriptDataOpts = config.extras.scriptDataOpts;
  b.shadowDomShim = config.extras.shadowDomShim;
  b.attachStyles = true;
+  b.invisiblePrehydration = typeof config?.invisiblePrehydration === 'undefined' ? true : config?.invisiblePrehydration;


Looking at the rest of this file, it looks like we don't use optional chaining for accessing the config. Is there a case you saw where we pass undefined to this function? Or can we simplify this to

Suggested change

b.invisiblePrehydration = typeof config?.invisiblePrehydration === 'undefined' ? true : config?.invisiblePrehydration;

b.invisiblePrehydration = typeof config.invisiblePrehydration === 'undefined' ? true : config.invisiblePrehydration;

I'm good with that. I'll try it out locally and if it breaks for some reason I'll revert it and let you know.

Worked great!

rwaskiewicz · 2021-08-18T14:21:14Z

src/runtime/bootstrap-lazy.ts

@@ -158,7 +158,7 @@ export const bootstrapLazy = (lazyBundles: d.LazyBundlesRuntimeData, options: d.
    })
  );

-  if (BUILD.hydratedClass || BUILD.hydratedAttribute) {
+  if (BUILD.invisiblePrehydration && (BUILD.hydratedClass || BUILD.hydratedAttribute)) {


For my own understanding, is this value always true, correct? IE This build flag just enables the feature? Or do the build conditionals get merged back into BUILD (and if so, where?)

Correct, invisiblePrehydration as a concept is true until you opt-out by setting that in consumers stencil.config.ts. I need to do more research on this BUILD concept, but I believe it's the latter right now. brb.

Interestingly topical to the rollup plugin discussion around source maps I posted a bit ago.

These get flattened within the appendBuildConditionals during the rollup plugin process.

So the BUILD that would be used in runtime is produced there.

TIL 👍 👍 👍

src/declarations/stencil-public-compiler.ts

rwaskiewicz · 2021-08-18T14:26:38Z

test/karma/stencil.config.ts

@@ -13,6 +13,7 @@ export const config: Config = {
  outputTargets: [
    {
      type: 'www',
+      empty: false,


For my own information, what does empty do (in general)? I don't think we have that documented on the stencil site ATM. Does it just empty the output directory if set to true?

empty: true (which is the default) on the www target will remove the directory prior to a build being run. This stop that from happening. Important because I wanted the sibling build to empty the directory, since it's the first build task in this process. That way, nothing in previous build processes get removed and are accessible for tests.

test/karma/test-app/invisible-prehydration-default/index.html

rwaskiewicz · 2021-08-18T15:49:22Z

test/karma/test-invisible-prehydration/package-lock.json

@@ -0,0 +1,10 @@
+{


FYI - the other packages in test/karma use the older version of lock files (v1) so the can be run in an npm 6/node 12 environment. This shouldn't really matter since you're not npm installing anything for this, but something to keep in mind (and I imagine when Node 12 support is dropped (in a distant breaking change)) we'll want to upgrade those v1 ones to v2

@splitinfinities: ADR this^^

Added in #3028

test/karma/test-invisible-prehydration/tsconfig.json

rwaskiewicz · 2021-08-18T16:04:21Z

test/karma/tsconfig.json

+    "test-app/global.ts",
+    "test-prehydrated-styles/components.d.ts",
+    "test-prehydrated-styles/**/*.spec.ts",
+    "test-prehydrated-styles/**/*.tsx"


For my own understanding, why do we need the top level tsconfig file to handle the transpilation too? I thought that was being handled bu the test-prehydrated-styles/tsconfig.json setup

Actually, does this directory exist? I don't see it added in this PR

This is a typo, it should be prehydration. And actually... not sure if it's needed since the tests pass. I'm going to review this.

@splitinfinities whatever came of that investigation? Do we need this?

Because this directory is built, then those built files are relocated, and all the tests associated are placed in the test-app directory, no we do not need this chunk. Deleted!

test/karma/test-invisible-prehydration/tsconfig.json

rwaskiewicz · 2021-08-18T17:38:27Z

test/karma/test-invisible-prehydration/stencil.config.ts

+    {
+      type: 'www',
+      dir: '../www',
+      empty: false,
+      indexHtml: 'prehydrated-styles.html',
+      serviceWorker: null,
+    },
+    {
+      type: 'www',
+      empty: false,
+      indexHtml: 'prehydrated-styles.html',
+      serviceWorker: null,


I'm not sure I fully understand why we need both of these output targets - would you mind explaining the usage of each to me?

My intent here was to allow for local debugging if necessary, and for placing the build content within the testable directory. It may seem clunky, and it is, but it's not as bad as it seems since this is only for putting the built files where the tests were already running.

I've got a good amount of brain fog ATM - but don't we have that ability (local debugging) from running npm start in the karma directory?

Hmmm so it looks like we don't serve the HTML in the local dev server, just the minified JS. Maybe that's what you were getting at?

I see what I did now. Originally, I was trying to allow tests within this directory. After trial and error, I kept this output target, but later I had moved the html file to the test-app directory, to work better with the karma automation.

Looking at this now, I think it's best to nix the local output target. I don't think there's much value in allowing these to be run directly, but if there was value, I would relocate these to the /test directory.

Merge when ionic-team/stencil#2989 is merged.

splitinfinities · 2021-08-24T17:50:11Z

Added ionic-team/stencil-site#761 for the stencil site

rwaskiewicz · 2021-08-25T12:37:40Z

test/karma/test-app/invisible-prehydration-default/karma.spec.ts

+
+  it('the style element will not be placed in the head', async () => {
+    tearDownStylesScripts();
+    await setupDom('/invisible-prehydration-default/index.html', 1000);


Double checking, is 1000 the best value here? I'm OK with it if it's the only way to avoid a flaky test, but waiting a full second for a test if we don't need it isn't ideal (this may apply to the other tests for prehyrdation as well)

I reduced each of these to 350, which seemed to be the most stable across the 6 tests I ran.

Ahhh, maybe that's because of my beefy processor. Seems I'm introducing 3 new potentially flakey tests... what's your perspective on that?

Let's bring it back up to 1000 for now, I tried locally w/500 and got just-okay consistency

rwaskiewicz · 2021-08-25T12:38:12Z

test/karma/test-app/invisible-prehydration-false/karma.spec.ts

+
+  it('the style element will not be placed in the head', async () => {
+    tearDownStylesScripts();
+    let app = await setupDom('/invisible-prehydration-false/index.html', 1000);


Suggested change

let app = await setupDom('/invisible-prehydration-false/index.html', 1000);

await setupDom('/invisible-prehydration-false/index.html', 1000);

rwaskiewicz · 2021-08-25T12:39:38Z

test/karma/test-app/invisible-prehydration-true/karma.spec.ts

+
+  it('the style element will not be placed in the head', async () => {
+    tearDownStylesScripts();
+    let app = await setupDom('/invisible-prehydration-true/index.html', 1000);


Suggested change

let app = await setupDom('/invisible-prehydration-true/index.html', 1000);

await setupDom('/invisible-prehydration-true/index.html', 1000);

rwaskiewicz · 2021-08-25T13:01:39Z

test/karma/tsconfig.json

@@ -26,7 +26,8 @@
    "paths": {
      "@stencil/core": ["../../internal"],
      "@stencil/core/internal": ["../../internal"],
-      "@stencil/core/testing": ["../../testing"]
+      "@stencil/core/testing": ["../../testing"],
+      "@test-invisible-prehydration": ["./test-invisible-prehydration"]


Do we use this path anywhere in the code? I don't see @test-invisible-prehydration anywhere in this PR

It is not, nixed

rwaskiewicz · 2021-08-25T13:02:44Z

test/karma/tsconfig.json

@@ -35,7 +36,10 @@
    "test-app/**/*.tsx",
    "test-app/**/*.ts",
    "test-app/util.ts",
-    "test-app/global.ts"
+    "test-app/global.ts",
+    "test-invisible-prehydration/components.d.ts",


I think these would be covered by the tsconfig file in the test-invisible-prehydration directory and can be removed

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

splitinfinities · 2021-08-27T16:38:04Z

Rebased with the leading branch, back up for review!

splitinfinities self-assigned this Aug 11, 2021

splitinfinities requested a review from a team August 11, 2021 13:52

splitinfinities marked this pull request as ready for review August 11, 2021 13:52

splitinfinities commented Aug 11, 2021

View reviewed changes

splitinfinities force-pushed the STENCIL-23-allow-disabling-hydration-stylesheets branch from a79104e to 1575230 Compare August 11, 2021 14:41

splitinfinities mentioned this pull request Aug 11, 2021

Rehydration of prerendered page results in flashing of some components #2245

Closed

paulcpederson mentioned this pull request Aug 11, 2021

Bug: flash of default styles due to Stencil bug with hydratedFlag Esri/calcite-design-system#991

Open

splitinfinities mentioned this pull request Aug 18, 2021

Slot content flickering when we working with visibility #3008

Closed

rwaskiewicz suggested changes Aug 18, 2021

View reviewed changes

splitinfinities force-pushed the STENCIL-23-allow-disabling-hydration-stylesheets branch from ecbee5b to b8f679c Compare August 24, 2021 17:10

splitinfinities added the work-in-progress: please review label Aug 24, 2021

splitinfinities added a commit to ionic-team/stencil-site that referenced this pull request Aug 24, 2021

Update config to document invisiblePrehydration

1ad78dd

Merge when ionic-team/stencil#2989 is merged.

splitinfinities mentioned this pull request Aug 24, 2021

Update config to document invisiblePrehydration ionic-team/stencil-site#761

Merged

rwaskiewicz reviewed Aug 25, 2021

View reviewed changes

splitinfinities requested review from rwaskiewicz and a team August 27, 2021 16:36

splitinfinities and others added 8 commits August 27, 2021 11:37

Allow disabling the injected hydration stylesheet

4f3e6bd

Adding invisiblePrehydration Karma tests

27c56f4

Updates from my own review

bf5c76d

Prettier fixup

987d193

Feedback from Ryan

8d242e7

Update test/karma/test-app/invisible-prehydration-default/karma.spec.ts

a315824

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Update index.html

82e96f6

Feedback from Ryan

b53904a

splitinfinities force-pushed the STENCIL-23-allow-disabling-hydration-stylesheets branch from 22f15be to b53904a Compare August 27, 2021 16:37

Patch test timing back up to 1 second

cf1ea47

rwaskiewicz approved these changes Aug 27, 2021

View reviewed changes

rwaskiewicz merged commit a3d2928 into master Aug 27, 2021

rwaskiewicz deleted the STENCIL-23-allow-disabling-hydration-stylesheets branch August 27, 2021 17:11

rwaskiewicz added Resolution: Shipped and removed work-in-progress: please review labels Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compiler): Allow disabling the injected hydration stylesheet #2989

feat(compiler): Allow disabling the injected hydration stylesheet #2989

splitinfinities commented Aug 4, 2021 •

edited

Loading

splitinfinities left a comment

rwaskiewicz Aug 18, 2021

splitinfinities Aug 19, 2021

splitinfinities Aug 23, 2021

rwaskiewicz Aug 18, 2021

splitinfinities Aug 19, 2021

splitinfinities Aug 19, 2021

rwaskiewicz Aug 24, 2021

rwaskiewicz Aug 18, 2021

splitinfinities Aug 19, 2021

rwaskiewicz Aug 18, 2021

splitinfinities Aug 23, 2021

splitinfinities Aug 27, 2021

rwaskiewicz Aug 18, 2021

rwaskiewicz Aug 18, 2021

splitinfinities Aug 19, 2021

rwaskiewicz Aug 24, 2021

splitinfinities Aug 27, 2021 •

edited

Loading

rwaskiewicz Aug 18, 2021

splitinfinities Aug 19, 2021

rwaskiewicz Aug 24, 2021

rwaskiewicz Aug 25, 2021

splitinfinities Aug 27, 2021

splitinfinities commented Aug 24, 2021

rwaskiewicz Aug 25, 2021 •

edited

Loading

splitinfinities Aug 27, 2021

splitinfinities Aug 27, 2021

rwaskiewicz Aug 27, 2021

rwaskiewicz Aug 25, 2021

rwaskiewicz Aug 25, 2021

rwaskiewicz Aug 25, 2021

splitinfinities Aug 27, 2021

rwaskiewicz Aug 25, 2021

splitinfinities commented Aug 27, 2021

	b.invisiblePrehydration = typeof config?.invisiblePrehydration === 'undefined' ? true : config?.invisiblePrehydration;
	b.invisiblePrehydration = typeof config.invisiblePrehydration === 'undefined' ? true : config.invisiblePrehydration;

	let app = await setupDom('/invisible-prehydration-false/index.html', 1000);
	await setupDom('/invisible-prehydration-false/index.html', 1000);

feat(compiler): Allow disabling the injected hydration stylesheet #2989

feat(compiler): Allow disabling the injected hydration stylesheet #2989

Conversation

splitinfinities commented Aug 4, 2021 • edited Loading

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

splitinfinities left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitinfinities Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitinfinities commented Aug 24, 2021

rwaskiewicz Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitinfinities commented Aug 27, 2021

splitinfinities commented Aug 4, 2021 •

edited

Loading

splitinfinities Aug 27, 2021 •

edited

Loading

rwaskiewicz Aug 25, 2021 •

edited

Loading