-
Notifications
You must be signed in to change notification settings - Fork 12
Update README.md #15
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
base: master
Are you sure you want to change the base?
Update README.md #15
Conversation
added reference to webreflection polyfills
README.md
Outdated
and it provides a bunch of workarounds for the spec rules involving class | ||
constructors and the `new` keyword. You should use it, too! | ||
Unfortunately the custom element polyfill provided by `@webcomponents` does not support extended built-in custom elements, has this [known limitations](https://github.com/webcomponents/custom-elements#known-bugs-and-limitations) and [**is slow**](https://github.com/webcomponents/custom-elements#customelementspolyfillwrapflushcallback). |
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 would object to this wording. The "slow" link references a feature to deviate from spec-compliance for better performance. There's no evidence given here that other polyfills are faster, or that they've even considered this case. If they do synchronous upgrade, then they're just as "slow". If the do async upgrade, they're not spec compliant. If the have an option, then they're equivalent to the @webcomponents
polyfill. Basically, this is hearsay.
Also, while the @webcomponents
polyfills do indeed have a detailed known issues section, that doesn't mean that other polyfills are without their known issues (a fully faithful polyfill might not be the right choice in all cases after all). Indeed, in the text below there's already a "constructor caveat" which, honestly, is a huge problem if I read it correctly. It seems to mean that most standard custom elements won't work because they won't have their constructor written in such a specific way. There's also a "caveats" section: https://github.com/WebReflection/document-register-element#common-issues--caveat Pointing out the @webcomponts
known issues without pointing out the others comes across as disingenuous.
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.
@justinfagnani
Thanks for your opinion.
I'm sorry that you feel this is hearsay. Please correct me, but the link I attached clearly states:
The polyfill gets slower as the size of your page and number of custom element definitons increases. You can use polyfillWrapFlushCallback to prevent redundant work.
You are right, also WebReflection's polyfills comes with caveats.
That's why I mentioned them too, as you can see below:
please note the
constructor
caveat ...
My purpose here is to be objective and show the good and bad parts clearly.
May I ask, do I touch work of yours?
I mean do I criticize work you where involved?
This is true because the spec states that upgrades must be synchronous. That same spec applies to all polyfills, so the real question is "slow compared to what". If you have reason to believe that the other polyfill avoids this slowdown, that would be good to point out, but otherwise there's no reason to believe that the other polyfill is faster in this regard.
This really didn't come across as objective, because the claims are fairly unsupported and asymmetrical: they imply that other polyfills don't have similar issues simply because they don't list them. |
@justinfagnani Well that point is best shown objectively by a benchmark. This is my humble experience with using both polyfills. Of course I'm absolutely open to a more gentle rewording. |
@justinfagnani PS: |
@justinfagnani |
I would recommend removing any mentions of slowness from here, until we have a proper benchmark. |
Thanks for your recommendation @web-padawan I agree, that a proper benchmark is the ideal provision of objective information. From the viewpoint of a consumer and builder of custom elements, we have a very high demand for performance and information about these facts are essential for us. Personally I would prefer to not hold back this important fact. If you guys insist, I will not struggle to reword it again. |
This a personal judgement and shouldn't be formulated like that. The current wording is biased. Ideally, all the polyfills should by listed in the same manner, and their limitations too. |
@web-padawan Agree, I will try to find a more diplomatic wording. |
@web-padawan I hope you agree with the new wording. Please let me know. |
@web-padawan |
README.md
Outdated
based on top, and compatible with, the battle-tested [Custom Elements V0](http://w3c.github.io/webcomponents/spec/custom/), | ||
already used in production with projects such as [Google AMP HTML ⚡](https://github.com/ampproject/amphtml#amp-html-) and others. | ||
- [WebReflection/document-register-element] (supports V1 despites it's name) has [fantastic browser support](https://github.com/WebReflection/document-register-element#tested-on) and a [public test page](http://webreflection.github.io/document-register-element/test/), please note the [`constructor` caveat](https://github.com/WebReflection/document-register-element#v1-caveat). | ||
- [built-in-element](https://github.com/ungap/custom-elements-builtin) again fantastic browser support and [live test page](https://ungap.github.io/custom-elements-builtin/test/es5/), please note the [`constructor` caveat](https://github.com/ungap/custom-elements-builtin#constructor-caveat). |
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.
Would be nice to link for the browser support separately:
https://github.com/ungap/custom-elements-builtin#compatible-with-
So, we might want to rephrase "fantastic" to "evergreen" browser support in this case.
@AndyOGo added one comment, apart from that, I don't have any other suggestions. |
@web-padawan Great, thanks for your feedback. |
@shawnbot As far as I know I have addressed all concerns of @justinfagnani and @web-padawan |
added reference to webreflection polyfills
fixes #14