-
Notifications
You must be signed in to change notification settings - Fork 795
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
feat(runtime): proxy form associated custom element lifecycle callbacks #4939
Conversation
|
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 |
There was a problem hiding this 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
@alicewriteswrongs thanks for the review. I addressed your comment. |
aee4558
to
7ef6406
Compare
@HansClaasen It looks like the Karma tests are failing here for all browsers we test against:
These tests historically can be a bit of a pain to debug. To make it a bit easier, I suggest building the compiler ( The format check is also failing, but that should be an easy fix 🙂 |
7ef6406
to
7d48170
Compare
@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. |
There was a problem hiding this 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
7d48170
to
5f8ce33
Compare
There was a problem hiding this 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
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 stenciljs/core#4939. STENCIL-1022: Documentation - Form Associated Custom Lifecycle Callbacks
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 stenciljs/core#4939. STENCIL-1022: Documentation - Form Associated Custom Lifecycle Callbacks
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?
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.