Updates to get the tests passing again. #36
Conversation
bicknellr
changed the title
customElements exists.customElements exists.
|
I'm still trying to figure out why |
| @@ -61,6 +61,9 @@ | |||
| (() => { | |||
| 'use strict'; | |||
|
|
|||
| // Do nothing if `customElements` does not exist. | |||
| if (!window.customElements) return; | |||
There was a problem hiding this comment.
I think this should check for polyfilled also
There was a problem hiding this comment.
Isn't that also a case where you wouldn't want the native shim to run?
| @@ -12,6 +12,7 @@ | |||
| <head> | |||
| <title>Custom Elements: imports integration</title> | |||
| <script> | |||
| window.customElements = window.customElements || {}; | |||
There was a problem hiding this comment.
window.customElements = window.customElements || {forcePolyfill: true};
?
Only cause this snippet might be what we recommend to users...
There was a problem hiding this comment.
If we do that, then forcePolyfill doesn't get added in the case that customElements already exists (native).
There was a problem hiding this comment.
How about
(window.customElements = window.customElements || {}).forcePolyfill = true; ?
There was a problem hiding this comment.
(I went ahead and changed to this.)
bicknellr
changed the title
customElements exists.customElements exists.
… match native, rather than being specifically true. They should be true per spec but it seems like Safari sets writable to false.
bicknellr
changed the title
customElements exists.
| @@ -13,9 +13,11 @@ suite('patching', function() { | |||
| suite('HTMLElement', function () { | |||
|
|
|||
| test('constructor is configurable and writable', function() { | |||
There was a problem hiding this comment.
This test doesn't really make sense. On this line, the new HTMLElement.prototype is set to the old one, so the descriptor is exactly the same. So, I'm going to remove this.
|
@justinfagnani Aside from a couple of unsupported browsers erroring out completely in WCT, I think this is ready for review. |
| @@ -16,14 +16,16 @@ | |||
| "url": "https://github.com/webcomponents/custom-elements/issues" | |||
| }, | |||
| "scripts": { | |||
| "install": "$(npm bin)/bower install", | |||
There was a problem hiding this comment.
This causes things to break for me if I npm install @webcomponents/custom-elements (I tried installing your forked version of custom-elements). Since bower is a devDependency, it is not installed but the install script still tries to use it.
What is the purpose of this install script? When somebody is npm installing this project, they don't need the bower_components, do they?
window.customElementsdoes not exist.customElementsto exist.String#endsWith, which doesn't get compiled during testing...npm installnow causesbower install.HTMLElement#constructordescriptor has bothwritableandconfigurabletrue was changed to check that these values match the native descriptor's values.