feat(compiler): add support for form-associated custom elements by alicewriteswrongs · Pull Request #4784 · stenciljs/core

alicewriteswrongs · 2023-09-12T20:24:06Z

This adds support for building form-associated custom elements in Stencil components, allowing Stencil components to participate in HTML forms in a rich manner. This is a popular request in the Stencil community (see #2284).

A minimal Stencil component that uses the new APIs to integrate with a form could look like this:

import { Component, h, AttachInternals } from '@stencil/core';

@Component({
  tag: 'my-component',
  styleUrl: 'my-component.css',
  shadow: true,
  formAssociated: true
})
export class MyComponent {
  @AttachInternals() internals: ElementInternals;

  render() {
    return <div>Hello, World!</div>;
  }
}

A few things to note:

A new option called formAssociated has been added to the ComponentOptions interface.
A new @AttachInternals() decorator can be used to indicate a property on a Stencil component to which an ElementInternals object will be bound at runtime.
Using @AttachInternals() is supported both for lazy builds (www, dist) as well as for dist-custom-elements.

The new behavior is implemented at compile-time, and so should result in only very minimal increases in code / bundle size.

Note

Browser support for the APIs that this feature depends on is still relatively low (~84% as of this writing) and the Stencil team does not plan to officially support or incorporate any polyfills for the browser functionality. Accordingly, this is a 'use at your own risk' feature: it is up to you as a developer to ensure the browsers you need to support have shipped the necessary APIs.

What is the current behavior?

Stencil does not support form-associated custom elements! Forms and Stencil components just aren't talking to each other as much as they could be.

What is the new behavior?

The Stencil compiler now supports emitting form-associated custom elements in the lazy and 'native' output targets.

Does this introduce a breaking change?

Yes
No

Testing

There are new tests which cover most of the functionality here.

There's an example Stencil project making use of these APIs here: https://github.com/alicewriteswrongs/stencil-element-internals-example

Other information

github-actions · 2023-09-12T20:25:34Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1399 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/mock-doc/serialize-node.ts	36
src/dev-server/server-process.ts	32
src/compiler/build/build-stats.ts	27
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts	25
src/compiler/style/test/optimize-css.spec.ts	23
src/testing/puppeteer/puppeteer-element.ts	23
src/compiler/prerender/prerender-main.ts	22
src/runtime/vdom/vdom-render.ts	20
src/runtime/client-hydrate.ts	19
src/screenshot/connector-base.ts	19
src/compiler/config/test/validate-paths.spec.ts	16
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/runtime/vdom/vdom-annotations.ts	14
src/sys/node/node-sys.ts	14
src/compiler/build/build-finish.ts	13
src/compiler/prerender/prerender-queue.ts	13

Our most common errors

Typescript Error Code	Count
TS2345	424
TS2322	398
TS18048	310
TS18047	100
TS2722	38
TS2532	34
TS2531	23
TS2454	14
TS2352	13
TS2769	10
TS2790	10
TS2538	8
TS2344	5
TS2416	4
TS2493	3
TS18046	2
TS2684	1
TS2488	1
TS2430	1

Unused exports report

There are 12 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/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	62	satisfies
src/compiler/types/validate-primary-package-output-target.ts	62	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

src/declarations/stencil-private.ts

src/compiler/transformers/static-to-meta/form-associated.ts

src/compiler/transformers/test/parse-form-associated.spec.ts

src/compiler/transformers/decorators-to-static/form-internals-decorator.ts

src/compiler/transformers/remove-static-meta-properties.ts

src/compiler/transformers/test/transpile.ts

src/runtime/bootstrap-lazy.ts

src/compiler/transformers/test/parse-form-associated.spec.ts

alicewriteswrongs

just pointing out one thing. I've cleanup up a few TODO items and little things here and there, I think this is ready for a more thorough review

src/runtime/bootstrap-lazy.ts

tanner-reits

All-in-all looks good imo. Noted a couple things, but no blockers!

src/compiler/transformers/component-lazy/form-internals.ts

src/compiler/transformers/decorators-to-static/form-internals-decorator.ts

src/compiler/transformers/component-native/form-internals.ts

src/runtime/bootstrap-lazy.ts

rwaskiewicz

All in all looks good! Architecturally I think this looks great!

I took this for a spin this morning, and ran into a few things that I wanted to run by you/the team. Some are commented inline, others are higher level and are below in the summary here:

Unit testing

When unit testing a stencil component starter (npm init stencil@latest component) and updating the component to support element internals:

I get the following error running tests on the project (npm t):

 FAIL  src/components/my-component/my-component.spec.ts
  ● Console

    console.log
      undefined

      at MyComponent.getText (src/components/my-component/my-component.tsx:30:13)

    console.log
      undefined

      at MyComponent.getText (src/components/my-component/my-component.tsx:30:13)

  ● my-component › renders

    TypeError: hostRef.$hostElement$.attachInternals is not a function

       8 |     formAssociated: true
       9 |   }
    > 10 | })
         |   ^
      11 | export class MyComponent {
      12 |   @FormInternals() internals: ElementInternals;
      13 |

      at new MyComponent (src/components/my-component/my-component.tsx:10:44)
      at initializeComponent (node_modules/@stencil/core/internal/testing/index.js:581:5)

  ● my-component › renders with values

    TypeError: hostRef.$hostElement$.attachInternals is not a function

       8 |     formAssociated: true
       9 |   }
    > 10 | })
         |   ^
      11 | export class MyComponent {
      12 |   @FormInternals() internals: ElementInternals;
      13 |

      at new MyComponent (src/components/my-component/my-component.tsx:10:44)
      at initializeComponent (node_modules/@stencil/core/internal/testing/index.js:581:5)

Did you see this at all in your testing? Anything abundantly clear that I'm doing wrong here?

HMR console errors

In testing this with the dev server, it looks like on every reload we get a DOMException: NotSupportedError as a result of trying to call AttachInternals() again. In the video below, I have the component that we generate from running npm init stencil@latest component, adapted to use formAssociated and @FormInternals (you should be able to see the changes I made to the source file in the left gutter of the editor there):

Screen.Recording.2023-09-15.at.8.48.08.AM.mov

I also tried:

Adding a second copy of this component to index.html - it appears we'll get an error for each component in the DOM
I verified this happens when a component does/doesn't have a parent <form>

Karma Testing

Could we add a Karma test that verifies that we can compile a component that uses form internals & capture some form data coming from a custom element?

src/compiler/transformers/decorators-to-static/component-decorator.ts

src/compiler/transformers/decorators-to-static/decorators-constants.ts

alicewriteswrongs · 2023-09-15T15:35:07Z

on the higher-level things:

testing

I think we'll need to probably add an element internals polyfill to mock-doc in order to support this stuff in unit tests. I'll look into adding this today.

With Karma tests I was planning to add a test suite for all this behavior but held off until the rest of the code was stabilized and whatnot, so likewise I'll dig into that today too.

HMR stuff

I've run into that error during local dev myself, essentially it's because you can't attachInternals on an element more than once (and, as you noted, it doesn't matter if an element is in a form or not since the browser lets you attachInternals anywhere afaict). I'll look into what we can do to mitigate this so you don't get an error when hot-reloading stuff happens.

alicewriteswrongs · 2023-09-20T15:07:53Z

@rwaskiewicz @tanner-reits I managed to figure out the Karma issue, basically I ended up just adding $hostElement$ to a do-not-mangle list in the options we pass to Terser. Possibly there's a better way...

alicewriteswrongs · 2023-09-25T19:13:58Z

Alright I also just added an "implementation" of attachInternals for mock-doc which returns a proxy that will log "hey this isn't implemented" if you try to access any properties on it. Here's what it looks like if you try to access something on it in a spec test:

Screen.Recording.2023-09-25.at.3.13.05.PM.mov

christian-bromann · 2023-09-29T16:04:07Z

test/karma/test-app/form-associated/karma.spec.ts

+  // it('should link up to the surrounding form', async () => {
+  //   const formEl = app.querySelector("form");
+  //   await waitForChanges();
+  //   const formData = new FormData(formEl);
+  //   expect(formData.get("test-input")).toBe("my default value");
+  // });


Is there a specific reason for having this code commented out?

just haven't gotten it working yet! I need to circle back once I've sorted out the HMR stuff

alicewriteswrongs · 2023-10-06T20:11:13Z

Ok so I haven't yet gone through the whole diff with a fine-toothed comb to make sure that all testing code and temporary changes and whatnot are all removed, but as of right now I think all of the functionality stuff I needed to get in place is finished. So HMR should now be fully supported! 🎉 So while I don't think this is quite ready for another close code review I do think it's ready for more functional testing

alicewriteswrongs

noting one thing

alicewriteswrongs · 2023-10-09T13:48:08Z

src/compiler/transformers/static-to-meta/component.ts

@@ -55,64 +57,49 @@ export const parseStaticComponentMeta = (
  const isCollectionDependency = moduleFile.isCollectionDependency;
  const encapsulation = parseStaticEncapsulation(staticMembers);
  const cmp: d.ComponentCompilerMeta = {


the diff here looks scarier than it is, I only actually changed two lines but the object wasn't alphabetized

one Q for reviewers: do we think this is safe? It's possible that some of the helper methods here have side-effects which would mean that we can't assume it's possible to safely re-order them

Could we pull this change out into a separate pull request? That ought to help us keep the scope of this small(er) and that way, we can worry about that separate from the element internals implementation work.

FWIW, I think one could argue that this was fine in the order it was originally in (at least in part) - the left hand side keeps things like class member-related items together. I'm not opposed to the change, just want to throw it out there that alphabetizing is something we should do judiciously as opposed to as the default (I've seen us as a team do that in a few PRs now).

I ended up backing that change out already as I was trying to debug the karma issues, pushing that code up now

alicewriteswrongs · 2023-10-09T17:54:12Z

Ok I believe this is now ready for code review. There are some issues with the Karma tests which I'm working on ironing out right now, but I think the code should still be ready for review

rwaskiewicz

Small number of asks - I haven't battle tested this yet, I'll do that once the dust has settled on Karma tests

rwaskiewicz · 2023-10-09T16:44:21Z

src/compiler/transformers/component-lazy/attach-internals.ts

+ * lazy-load ready Stencil component.
+ *
+ * In order to create a lazy-loaded form-associated component we need to access
+ * the underlying host element (via the "$hostElement$" prop on {@link d.hostRef})


should this be:

Suggested change

* the underlying host element (via the "$hostElement$" prop on {@link d.hostRef})

* the underlying host element (via the "$hostElement$" prop on {@link d.HostRef})

hmm I wish something would throw an error if those @link bits didn't resolve correctly :/

rwaskiewicz · 2023-10-09T17:35:59Z

src/compiler/transformers/decorators-to-static/decorator-utils.ts


 import { objectLiteralToObjectMap } from '../transform-utils';
-import type { StencilDecorator } from './decorators-constants';
+import { StencilDecorator } from './decorators-constants';


It looks like we only use StencilDecorator as a type in this file - is there a particular reason we remove the type keyword from the import statement?

rwaskiewicz · 2023-10-09T17:45:25Z

src/compiler/transformers/static-to-meta/component.ts

@@ -55,64 +57,49 @@ export const parseStaticComponentMeta = (
  const isCollectionDependency = moduleFile.isCollectionDependency;
  const encapsulation = parseStaticEncapsulation(staticMembers);
  const cmp: d.ComponentCompilerMeta = {


Could we pull this change out into a separate pull request? That ought to help us keep the scope of this small(er) and that way, we can worry about that separate from the element internals implementation work.

FWIW, I think one could argue that this was fine in the order it was originally in (at least in part) - the left hand side keeps things like class member-related items together. I'm not opposed to the change, just want to throw it out there that alphabetizing is something we should do judiciously as opposed to as the default (I've seen us as a team do that in a few PRs now).

src/runtime/bootstrap-lazy.ts

alicewriteswrongs · 2023-10-09T19:16:30Z

some documentation up here: stenciljs/site#1247

tanner-reits

Code-wise, looks solid! Haven't had a change to test it out yet, so will get around to that asap!

tanner-reits · 2023-10-09T20:04:25Z

src/mock-doc/node.ts

  __style: MockCSSStyleDeclaration | null | undefined;

+  attachInternals(): MockElementInternals {
+    return new Proxy({} as unknown as MockElementInternals, {


tanner-reits · 2023-10-09T20:16:12Z

src/compiler/transformers/component-lazy/attach-internals.ts

appreciate all the documentation here!

alicewriteswrongs · 2023-10-12T19:45:06Z

alright at long last the build is fully passing here, so I believe this is all ready for a code review!

rwaskiewicz

Looks good! I had one non-blocking question, and found one bug that I think we can work on next week (and don't need to handle just this second).

Video of the bug in question below. In it, I have the output of npm init stencil@latest component with this branch installed. When I flip the bit on formAssociated, I get a few console errors in the browser:

Screen.Recording.2023-10-12.at.4.18.37.PM.mov

rwaskiewicz · 2023-10-12T19:59:37Z

test/karma/stencil.config.ts

  namespace: 'TestApp',
  srcDir: 'test-app',
  tsconfig: 'tsconfig-stencil.json',
+  sourceMap: false,


Were sourcemaps messing with the mangling?

I think so, although I think I just inadvertently left that change in after fixing the real issue - I flipped it back and just pushed that up

alicewriteswrongs · 2023-10-12T21:01:35Z

@rwaskiewicz does that only happen with HMR or on first page load too?

rwaskiewicz · 2023-10-12T21:04:42Z

@alicewriteswrongs it always happens in HMR when the value goes from false to true. That is, if I set the value to false before I start the dev server, start the dev server, then switch it to true, I get that error. I also get that error if I have the value set to true, start the dev server, then set the value to false then true again (it only happens on the false -> true transition)

tanner-reits

Looks good!

This adds support for building form-associated custom elements in Stencil components, allowing Stencil components to participate in HTML forms in a rich manner. This is a popular request in the Stencil community (see #2284). The new form-association technology is exposed to the component author via the following changes: - A new option called `formAssociated` has been added to the [`ComponentOptions`](https://github.com/ionic-team/stencil/blob/06f6fad174c32b270ce239afab5002c23d30ccbc/src/declarations/stencil-public-runtime.ts#L10-L55) interface. - A new `@AttachInternals()` decorator can be used to indicate a property on a Stencil component to which an [`ElementInternals`](https://developer.mozilla.org/en-US/docs/Web/API/ElementInternals) object will be bound at runtime. - Using `@AttachInternals()` is supported both for lazy builds ([`www`](https://stenciljs.com/docs/www), [`dist`](https://stenciljs.com/docs/distribution)) as well as for [`dist-custom-elements`](https://stenciljs.com/docs/custom-elements). The new behavior is implemented at compile-time, and so should result in only very minimal increases in code / bundle size. Support exists for using form-associated components in both the lazy and the CE output targets, as well as some extremely minimal provisions for testing. Documentation for this feature was added to the Stencil site here: stenciljs/site#1247

Form-associated custom elements were added to Stencil in stenciljs/core#4784 for version 4.5.0. This adds documentation explaining the changes to the component authoring DSL and also breaking down some examples of how the functionality could be used to build out a component.

alicewriteswrongs force-pushed the ap/element-internals-poc branch 2 times, most recently from b62ed68 to 464ef1f Compare September 13, 2023 18:33

alicewriteswrongs marked this pull request as ready for review September 13, 2023 18:35

alicewriteswrongs requested a review from a team as a code owner September 13, 2023 18:35

alicewriteswrongs requested review from rwaskiewicz and tanner-reits September 13, 2023 18:35

rwaskiewicz reviewed Sep 13, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/element-internals-poc branch 2 times, most recently from af5c47e to 02c50e9 Compare September 14, 2023 17:15

alicewriteswrongs commented Sep 14, 2023

View reviewed changes

src/runtime/bootstrap-lazy.ts Outdated Show resolved Hide resolved

alicewriteswrongs requested a review from rwaskiewicz September 14, 2023 17:18

tanner-reits approved these changes Sep 14, 2023

View reviewed changes

rwaskiewicz requested changes Sep 15, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/element-internals-poc branch 2 times, most recently from 54c41de to 122a9bd Compare September 15, 2023 15:23

alicewriteswrongs force-pushed the ap/element-internals-poc branch 3 times, most recently from d51af42 to 0931e64 Compare September 20, 2023 14:23

alicewriteswrongs force-pushed the ap/element-internals-poc branch 2 times, most recently from 7201415 to 827373a Compare September 25, 2023 18:03

alicewriteswrongs force-pushed the ap/element-internals-poc branch from a48e908 to baf5570 Compare September 29, 2023 15:02

christian-bromann reviewed Sep 29, 2023

View reviewed changes

danielleroux mentioned this pull request Oct 6, 2023

[IxSelect] Bad interoperability with react-hook-form siemens/ix#691

Closed

alicewriteswrongs force-pushed the ap/element-internals-poc branch 2 times, most recently from 2965076 to 1b7ac63 Compare October 6, 2023 18:27

alicewriteswrongs commented Oct 9, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/element-internals-poc branch from fb011f9 to fd268d3 Compare October 9, 2023 14:13

rwaskiewicz requested changes Oct 9, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/element-internals-poc branch from 55e24ba to 81a628c Compare October 9, 2023 18:40

alicewriteswrongs mentioned this pull request Oct 9, 2023

docs for form-associated custom elements in Stencil stenciljs/site#1247

Merged

tanner-reits reviewed Oct 9, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/element-internals-poc branch from 81a628c to 0e8548b Compare October 9, 2023 20:24

danielleroux mentioned this pull request Oct 10, 2023

Select element doesn't work properly with native HTML form siemens/ix#834

Closed

alicewriteswrongs force-pushed the ap/element-internals-poc branch from 0e8548b to bd92e5b Compare October 12, 2023 14:23

alicewriteswrongs requested review from rwaskiewicz and tanner-reits October 12, 2023 20:25

rwaskiewicz approved these changes Oct 12, 2023

View reviewed changes

tanner-reits approved these changes Oct 13, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/element-internals-poc branch 2 times, most recently from c6cc9af to d196b66 Compare October 16, 2023 13:59

alicewriteswrongs force-pushed the ap/element-internals-poc branch from d196b66 to 9f2360b Compare October 16, 2023 14:00

alicewriteswrongs enabled auto-merge October 16, 2023 14:01

alicewriteswrongs added this pull request to the merge queue Oct 16, 2023

Merged via the queue into main with commit 5976c9b Oct 16, 2023

alicewriteswrongs deleted the ap/element-internals-poc branch October 16, 2023 14:37

dgonzalezr mentioned this pull request Oct 17, 2023

chore(deps): update dependency @stencil/core to v4.5.0 Endava/BEEQ#584

Merged

1 task

	* the underlying host element (via the "$hostElement$" prop on {@link d.hostRef})
	* the underlying host element (via the "$hostElement$" prop on {@link d.HostRef})

Uh oh!

Conversation

alicewriteswrongs commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

Uh oh!

github-actions bot commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

--strictNullChecks error report

reports and statistics

Unused exports report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tanner-reits left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rwaskiewicz left a comment

Choose a reason for hiding this comment

Unit testing

HMR console errors

Karma Testing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alicewriteswrongs commented Sep 15, 2023

testing

HMR stuff

Uh oh!

alicewriteswrongs commented Sep 20, 2023

Uh oh!

alicewriteswrongs commented Sep 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alicewriteswrongs commented Oct 6, 2023

Uh oh!

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alicewriteswrongs commented Oct 9, 2023

Uh oh!

rwaskiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

alicewriteswrongs commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading

`--strictNullChecks` error report