-
Notifications
You must be signed in to change notification settings - Fork 11
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
Forcing custom commands to have --
prefix breaks polyfilling native commands
#44
Comments
Yeah this is an intentional part of the design and will be enforced by browsers/spec (when I get round to it 😉). See the resulting open-ui discussion here; openui/open-ui#969 (comment) which was a conclusion from WHATWG meeting here: whatwg/html#10471. The intent behind this is that prospective polyfills - until the behaviour has reasonably shipped - should use the the custom action syntax. So |
@keithamus I'd argue it's not really a polyfill if the HTML syntax has to change (i.e. we're having to use For example, if I completely understand and support the desire to avoid conflicts with future native commands, but from my perspective that can be made clear through the spec and devtools warnings without hampering adoption through lack of proper polyfill support. |
It'd hopefully be a very low priority refactor to go into even a large codebase and
The polyfill itself will eventually polyfill all the built-ins when they've gathered enough consensus. Shipping in even one browser is more than enough consensus for us to polyfill that one command. Right now I'd say it's too early to polyfill I plan to extend this repository to add more of the proposed built-ins, and provide an easier way to polyfill the proposed built-ns. Right now it's very precisely targeting v1 of the spec so we have a reasonable baseline. I welcome PRs for polyfilling the newer commands, provided:
|
Sorry, I think we're on slightly different wavelengths here and it might be down to my ignorance/naivety around how it'll be specced. I'll try to clarify and hopefully you can figure out where I'm wrong: First up, I completely agree re: speculation, I'm not suggesting the polyfill should include Secondly, you're absolutely right that it's an easy refactor to find-and-replace My concern is how we polyfill new native commands if the spec completely precludes using non-prefixed non-native commands. I guess this is where the wording of spec and the implementation of browsers comes in. For example, if the spec (in layman's terms) says something like:
Then that makes sense to me. But if the latter sentence was:
Then this would prevent us from polyfilling, as the So based on this... I'm assuming the spec will be along the line of the former? If so, all is well and I think the part that confused me is purely the polyfill's use of the |
I'll cite the spec here so we're all clear:
So step 2 says Step 3 days "if it's not build-in ("is in no state") and it's not custom ("isCustom is false") then return early". So it's the second of your two summaries. In this way the polyfill is following the spec exactly.
It's fine that the event is never dispatched, it's trivial to synthesize one. We can listen for the click event on a button, and - if the el.commandForElement.dispatchEvent(new CommandEvent('command', { source: el, command: el.command }) There's a small amount of extra work but it's not that difficult to polyfill. There will be a definite path to adoption. If in your scenario Chrome adopted
Each of these choices has trade-offs which need to be evaluated, but demonstrate quite succinctly that it's definitely not impossible to polyfill new commands. |
Thanks @keithamus – that's a brilliant write-up, I'd completely overlooked that we can just listen on It's a shame it's not quite as succinct as just listening on I should have definitely clarified this before publishing a video on the API... I've updated the write-up though. Really appreciate your time explaining it 🙏 |
I'm not sure whether this is intended to be enforced by the spec / browser implementation, but having this line:
invokers-polyfill/invoker.js
Line 282 in 15a020a
step-up
, for example, doesn't work.This may be more of a discussion for elsewhere, but I raised here incase it's just the polyfill's implementation of the 'rule', you'll know better than me @keithamus.
An option would be to remove the
return
here, so the warning occurs but the behaviour still works:invokers-polyfill/invoker.js
Lines 281 to 284 in 15a020a
The text was updated successfully, but these errors were encountered: