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

feat(runtime): proxy form associated custom element lifecycle callbacks #4939

Merged

Conversation

HansClaasen
Copy link
Contributor

What is the current behavior?

Currently a Stencil component can not make use of the following form associated custom element lifecycle callbacks:

  • formAssociatedCallback
  • formResetCallback
  • formDisabledCallback
  • formStateRestoreCallback

See more on these callbacks on https://web.dev/articles/more-capable-form-controls

GitHub Issue Number: N/A

What is the new behavior?

With this patch we update the proxy class to propagate the function calls to the component, if the Stencil component has a formAssociated flag set.

Does this introduce a breaking change?

  • Yes
  • No

Testing

I enhanced an e2e test to set the default aria label for the component to a specific value which I then assert in the test. For simplicity reasons I refrained adding a test for every supported callback given they are all proxied the same way. Happy to add the tests though if desired.

Other information

There might be things, e.g. missing types or compiler changes, that I might have overseen. Please point me to missing pieces if so.

Also I am not sure how much we want to optimize the compiler here. Currently code will added to the stencil core even though none of the components are using any of these form associated callbacks. I can add compiler flags for each one of them to ensure we actually ship a minimum necessary amount of code.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2023

--strictNullChecks error report

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

That's 7 fewer than on main! 🎉🎉🎉

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 23
src/compiler/style/test/optimize-css.spec.ts 23
src/testing/puppeteer/puppeteer-element.ts 23
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts 22
src/compiler/prerender/prerender-main.ts 22
src/runtime/client-hydrate.ts 19
src/screenshot/connector-base.ts 19
src/runtime/vdom/vdom-render.ts 18
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 403
TS2322 384
TS18048 303
TS18047 100
TS2722 38
TS2532 34
TS2531 23
TS2454 14
TS2352 13
TS2769 10
TS2790 10
TS2538 8
TS2416 6
TS2344 5
TS2493 3
TS2488 2
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

@alicewriteswrongs alicewriteswrongs self-assigned this Oct 17, 2023
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.

👋 thanks for this contribution!

I just took it for a quick test-drive and everything looks good, I was able to modify my form-associated example to use the formAssociatedCallback and it worked great!

I just noted a few things in the code

src/runtime/runtime-constants.ts Outdated Show resolved Hide resolved
@HansClaasen
Copy link
Contributor Author

@alicewriteswrongs thanks for the review. I addressed your comment.

@HansClaasen HansClaasen force-pushed the hc/element-lifecycle-callbacks branch 2 times, most recently from aee4558 to 7ef6406 Compare October 20, 2023 00:28
@rwaskiewicz
Copy link
Member

@HansClaasen It looks like the Karma tests are failing here for all browsers we test against:

Firefox 118.0 (Windows 10) form associated should trigger form associated custom element lifecycle callbacks FAILED
	Expected undefined to be 'formAssociated called'.
	<Jasmine>
	global.wrappers["/home/runner/work/stencil/stencil/test/karma/test-app/form-associated/karma.spec.ts"]</</</</<@test-app/form-associated/karma.spec.js:67:38
	step@test-app/form-associated/karma.spec.js:33:23
	verb/<@test-app/form-associated/karma.spec.js:14:53
	global.wrappers["/home/runner/work/stencil/stencil/test/karma/test-app/form-associated/karma.spec.ts"]</__awaiter</<@test-app/form-associated/karma.spec.js:8:71
	global.wrappers["/home/runner/work/stencil/stencil/test/karma/test-app/form-associated/karma.spec.ts"]</__awaiter<@test-app/form-associated/karma.spec.js:4:12
	global.wrappers["/home/runner/work/stencil/stencil/test/karma/test-app/form-associated/karma.spec.ts"]</</<@test-app/form-associated/karma.spec.js:63:98
	<Jasmine>

These tests historically can be a bit of a pain to debug. To make it a bit easier, I suggest building the compiler (npm run build) and navigating to test/karma. From there, you can start a Stencil dev server with npm start and navigate to http://localhost:3333/test-www/form-associated (the port may be different, check the output of starting the dev server to make sure it matches). From there, you should be able to poke at the component and start debugging.

The format check is also failing, but that should be an easy fix 🙂 npm run prettier

@HansClaasen
Copy link
Contributor Author

@rwaskiewicz thanks for the tip 👌 I was able to debug the issue and resolve it. The problem was that the prototype tweak was only applied if the host element had members. I moved the change outside of that if statement and it started to work for the test component.

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 think this looks good! There's some lint / formatting issues to resolve, but once that's fixed I think this is good to go

Copy link
Member

@tanner-reits tanner-reits left a comment

Choose a reason for hiding this comment

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

Think this looks good! Only ask would be to resolve some of the new SNC violations this introduced, but I won't consider that a blocker

@christian-bromann christian-bromann added this pull request to the merge queue Nov 17, 2023
Merged via the queue into ionic-team:main with commit ca53dbb Nov 17, 2023
120 checks passed
@christian-bromann christian-bromann deleted the hc/element-lifecycle-callbacks branch November 17, 2023 19:38
@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 Nov 20, 2023
alicewriteswrongs added a commit to ionic-team/stencil-site that referenced this pull request Nov 29, 2023
This adds documentation of the form-associated lifecycle callbacks,
explaining what they do and providing some quick examples of how they
can be used.

We added these to Stencil in
ionic-team/stencil#4939.

STENCIL-1022: Documentation - Form Associated Custom Lifecycle Callbacks
alicewriteswrongs added a commit to ionic-team/stencil-site that referenced this pull request Nov 29, 2023
This adds documentation of the form-associated lifecycle callbacks,
explaining what they do and providing some quick examples of how they
can be used.

We added these to Stencil in
ionic-team/stencil#4939.

STENCIL-1022: Documentation - Form Associated Custom Lifecycle Callbacks
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.

None yet

5 participants