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

Forcing custom commands to have -- prefix breaks polyfilling native commands #44

Closed
ryantownsend opened this issue Jul 31, 2024 · 6 comments

Comments

@ryantownsend
Copy link

I'm not sure whether this is intended to be enforced by the spec / browser implementation, but having this line:

`"${source.command}" is not a valid command value. Custom commands must begin with --`,
means polyfilling 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:

console.warn(
`"${source.command}" is not a valid command value. Custom commands must begin with --`,
);
return;

@keithamus
Copy link
Owner

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 step-up needs to be --step-up until such a time that we're sure a native step-up will ship.

@ryantownsend
Copy link
Author

@keithamus I'd argue it's not really a polyfill if the HTML syntax has to change (i.e. we're having to use --step-up not step-up)

For example, if step-up has shipped in Chromium but WebKit/Firefox were dragging their heels for 12 months... in order to actually use that native command, everyone would be forced back into the old school method of waiting not only for baseline support but months/years past that (to ensure all their end-users have upgraded) to adopt. So Chromium would see next to zero adoption of their new feature until developers can rely on it in every other browser.

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.

@keithamus
Copy link
Owner

@keithamus I'd argue it's not really a polyfill if the HTML syntax has to change (i.e. we're having to use --step-up not step-up)

It'd hopefully be a very low priority refactor to go into even a large codebase and s/--step-up/step-up. The -- syntax is for custom commands if you want to make up your own. For highly speculative commands that might never make it in to browsers I'd encourage using a custom command because speculatively polyfilling commands that don't really exist beyond a 1 line explainer note may cause months/years of headache for browser maintainers. Think of it like patching prototypes - only do it if you know the semantics 😉.

For example, if step-up has shipped in Chromium but WebKit/Firefox were dragging their heels for 12 months

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 step-up as IIRC it's not even in a single browser, nor does it have a spec, and there's no intent do push it forward right now (as focus is on getting v1 out).

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:

  • They're opt in (e.g. via a separate file or an additional function to run) OR
  • They're in at least 1 browser OR
  • There are some WPTs & specification prose in flight.

@ryantownsend
Copy link
Author

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 step-up at this stage – focusing on v1 makes complete sense.

Secondly, you're absolutely right that it's an easy refactor to find-and-replace --step-up to step-up – I've no problem in implementing the double-hyphen-prefix route until consensus has been reached on future native commands.

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:

As an author (developer), you must either stick to native commands OR double-hyphen-prefix your custom commands.

But as an implementer (browser), you must always send the CommandEvent regardless of whether the author adheres to the above rule or not (possibly issuing a warning if non-native commands aren't double-hyphen-prefixed)

Then that makes sense to me.

But if the latter sentence was:

As an implementer (browser), if the author has used a custom command that isn't double-hyphen-prefixed, then it should be ignored and/or trigger an error

Then this would prevent us from polyfilling, as the CommandEvent would never be triggered, so in a potential future scenario where Chromium had implemented step-up but the other browsers haven't, there's no path to adopting step-up other than waiting for months/years for baseline + end-users to upgrade.

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 return – if that were implemented in the browser, then it would break polyfilling, but in hindsight including it makes sense in the polyfill.

@keithamus
Copy link
Owner

My concern is how we polyfill new native commands if the spec completely precludes using non-prefixed non-native commands. [...] For example, if the spec (in layman's terms) says something like:

I'll cite the spec here so we're all clear:

  1. Let isCustom be true if the command attribute is in no state, and command's value is a valid custom command, and false otherwise.
  2. If the command attribute is in no state and isCustom is false, then return.

So step 2 says isCustom is true only if the commands value is not built-in (the "is in no state" bit) and is a "valid custom command" (which will be updated to say it needs to start with --).

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.

Then this would prevent us from polyfilling, as the CommandEvent would never be triggered, so in a potential future scenario where Chromium had implemented step-up but the other browsers haven't, there's no path to adopting step-up other than waiting for months/years for baseline + end-users to upgrade.

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 command is our polyfilled command - simply run

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 step-up and the others didn't, we can write a polyfill for just the step-up command in one of two ways:

  • Add a capturing event listener for command at the document root, that calls stopImmediatePropagation() && preventDefault() if the event .isTrusted (i.e. it's an event that comes direct from the browser). This we effectively disable native step-up in Chrome and rely on the polyfill implementation, and for browsers which don't dispatch the event they need the polyfill anyway. The upside to this is that the polyfill is consistently applied, the minor downsides is that .isTrusted will always be false while the polyfill is active, and it's not so much polyfilling as it is replacing functionality. Those are almost negligible downsides though.

  • Dispatch the polyfilled event after a microtask, but listen for the native event in the meantime and don't dispatch the polyfilled event if the native one fired. Something like...

    let nativeFired = false;
    el.addEventListener('command', () => (nativeFired = true), { once: true })
    await Promise.resolve() // wait 1 microtask
    if (!nativeFired) el.dispatchEvent(new CommandEvent('command', options))

    The upsides to this method is that the polyfill will not apply to browsers with the native functionality. The downside is that the polyfilled commands fire ever so slightly later than native (by 1 microtask).

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.

@ryantownsend
Copy link
Author

Thanks @keithamus – that's a brilliant write-up, I'd completely overlooked that we can just listen on click (as the polyfill literally does today... 🤦‍♂️)

It's a shame it's not quite as succinct as just listening on command and applying a patch there, but I appreciate that's a worthwhile trade-off to have a clear boundary for maintaining future compatibility in the spec 👍

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 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants