X-brower autonomous CEv1 #39

tomalec · 2017-12-13T11:25:04Z

Workaround imported scripts execution,
Starcounter/starcounter-include#69

⚠️ Change the expected behavior of dom-repeat and CE & scripts execution order
Due to HTML Imports v1 bug/inconsistency the order of onload callbacks (therefore stamping) of the same imported document (partial) is reversed (last to first)

Stop testing in IE due to html-imports issue
Start testing in Safari 10
Stop testing in Safari 9, as it's hard to investigate tests failures and I hope it's old enough to drop the support.

as in webcomponents/html-imports#77 webcomponents/webcomponentsjs#872 Starcounter/starcounter-include#69

and CE & scripts execution order

webcomponents/webcomponentsjs#872 Start testing in Safari 10

alshakero

I really love the recursive snippet, just minor comments from Skavsta-Stockholm bus 😄

alshakero · 2017-12-13T12:23:12Z

imported-template.html

+                            replaceScripts(template.content);
+                          });
+                        };
+                        replaceScripts(this.import);


Such a clean, super-readable snippet. Gotta compliment this.

alshakero · 2017-12-13T12:25:14Z

README.md

@@ -125,11 +125,15 @@ Name       | When                               | `event.detail`

 Browser support relies mainly on polyfills support/spec compliance.

-| Chrome | IE11 | Edge | Firefox  | Safari 9 |
+| Chrome | IE11 | Edge | Firefox  | Safari 9+ |


I think 9+ includes 9? Because 18+ includes 18 😄

👍 forgot to push another README change

alshakero · 2017-12-13T12:27:55Z

imported-template.html

+        // https://github.com/webcomponents/html-imports/pull/77
+        // is merged
+        const HTMLImportsV1PolyfillInUse = HTMLImports && (HTMLImports.useNative === false) && HTMLImports.importForElement;
+        const scriptsSelector = 'script:not([type]),script[type="application/javascript"],script[type="text/javascript"]';


I don't see a reason not to include all script types. If you have a reason you can ignore my comment.

Agree. <script src="javascript.js"></script> is valid HTML5:

Omitting the attribute, or setting it to a JavaScript MIME type, means that the script is a classic script, to be interpreted according to the JavaScript Script top-level production.

https://www.w3.org/TR/html5/semantics-scripting.html#the-script-element

@warpech yes this one in included. But this one is not <script type="application/json" />

To make it consistent with polyfill fix,

<script type="application/json" /> is not executed as JS, so it's not blocked by browsers, there is no need to clone it.

@warpech <script src="javascript.js"></script> is covered.

Ah right! Thanks for explanation

alshakero · 2017-12-13T12:31:03Z

imported-template.html

+                    // Make scripts from imported templates work in browsers polyfilled by wcjs#v1 HTML Imports
+                    if(HTMLImportsV1PolyfillInUse){
+                        const replaceScripts = (content) => {
+                          forEach(content.querySelectorAll('template'), template => {


Since you're using arrow functions (ES6), why not use native for...of? It iterates HTMLCollections and NodeLists and doesn't need hasOwnProperty,

In response to @alshakero: It is a copy paste from webcomponents/html-imports#77 and as such it is better to keep it intact or change it at the source.

Out of laziness. To use the same snippet as in HTML Imports to make maintenance easier ;)

tomalec · 2017-12-13T13:02:58Z

Is there anything left?

warpech · 2017-12-13T12:56:23Z

imported-template.html

@@ -72,6 +89,24 @@
                    that.scopelessNodes = [];
                    that.clear();

+                    // Make scripts from imported templates work in browsers polyfilled by wcjs#v1 HTML Imports


Please add a comment saying that this should be revised after webcomponents/html-imports#77 is relesaed.

Like one at https://github.com/Juicy/imported-template/pull/39/files/cf2267548157b82b9797fca37bd9cbe104362e8e#diff-5df4cf8262d2c40490e3787fbc31c2abR17 ?
Would you like to see a duplicate comment here as well?

Oh, I haven't noticed it. I would actually just move this comment to l. 92

Why not keep them higher than lower? If you read it at l.17 and realize it's no longer needed, you will start to remove those bits. I've added duplicated comment to make it even more explicit.

warpech · 2017-12-13T12:57:00Z

imported-template.html

+                    // Make scripts from imported templates work in browsers polyfilled by wcjs#v1 HTML Imports
+                    if(HTMLImportsV1PolyfillInUse){
+                        const replaceScripts = (content) => {
+                          forEach(content.querySelectorAll('template'), template => {


In response to @alshakero: It is a copy paste from webcomponents/html-imports#77 and as such it is better to keep it intact or change it at the source.

warpech · 2017-12-13T13:07:56Z

Due to HTML Imports v1 bug/inconsistency the order of onload callbacks (therefore stamping) of the same imported document (partial) is reversed (last to first)

So the order is different in Chrome and in the polyfilled browsers?

It this problem fixable in the polyfilled? If yes, is there an issue for it?

tomalec · 2017-12-13T18:25:37Z

So the order is different in Chrome and in the polyfilled browsers?

Yes

It this problem fixable in the polyfilled?

Hard to say right now.

If yes, is there an issue for it?

There is one mentioned in code comments and console.logs https://github.com/webcomponents/html-imports/issues/79

warpech · 2017-12-14T11:35:04Z

imported-template.html

@@ -14,6 +14,23 @@

            return (typeof HTMLImports !== 'undefined') && !HTMLImports.useNative && HTMLImports.parser && !isSafariWithWc;
        })();
+        // We neet to workaround exectuting scripts from imported tempalte until


-> We need to workaround executing scripts from imported-template until

tomalec · 2017-12-14T21:32:26Z

@warpech how about now?

warpech

Lgtm

warpech · 2018-01-09T21:43:03Z

Setting as “Finished” because it is:

released (with release notes) https://github.com/Juicy/imported-template/releases/tag/3.1.0
included in a Finished release of SCF https://github.com/Starcounter/StarcounterClientFiles/releases/tag/2.1.0

tomalec added 4 commits December 12, 2017 21:48

Workaround imported scripts execution,

7938db7

as in webcomponents/html-imports#77 webcomponents/webcomponentsjs#872 Starcounter/starcounter-include#69

Change the expected behavior of dom-repeat

47083ed

and CE & scripts execution order

Stop testing in IE due to html-imports issue

be8b70f

webcomponents/webcomponentsjs#872 Start testing in Safari 10

Stop testing in Safari 9

cf22675

tomalec added the bug label Dec 13, 2017

tomalec requested review from warpech and alshakero December 13, 2017 11:25

alshakero requested changes Dec 13, 2017

View reviewed changes

Update supported Safari version in the README

fd3bf6c

warpech suggested changes Dec 13, 2017

View reviewed changes

alshakero approved these changes Dec 13, 2017

View reviewed changes

warpech suggested changes Dec 14, 2017

View reviewed changes

Fix comments about html-imports workaround

e68ebc1

warpech approved these changes Dec 15, 2017

View reviewed changes

tomalec merged commit 9140137 into master Dec 15, 2017

tomalec deleted the x-brower-autonomous-CEv1 branch December 15, 2017 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

X-brower autonomous CEv1 #39

X-brower autonomous CEv1 #39

tomalec commented Dec 13, 2017

alshakero left a comment

alshakero Dec 13, 2017

alshakero Dec 13, 2017

tomalec Dec 13, 2017

alshakero Dec 13, 2017

warpech Dec 13, 2017 •

edited

Loading

alshakero Dec 13, 2017

tomalec Dec 13, 2017

tomalec Dec 13, 2017

warpech Dec 13, 2017

alshakero Dec 13, 2017

warpech Dec 13, 2017 •

edited

Loading

tomalec Dec 13, 2017

tomalec commented Dec 13, 2017

warpech Dec 13, 2017

tomalec Dec 13, 2017

warpech Dec 14, 2017

tomalec Dec 14, 2017

warpech Dec 13, 2017 •

edited

Loading

warpech commented Dec 13, 2017

tomalec commented Dec 13, 2017

warpech Dec 14, 2017

tomalec commented Dec 14, 2017

warpech left a comment

warpech commented Jan 9, 2018

X-brower autonomous CEv1 #39

X-brower autonomous CEv1 #39

Conversation

tomalec commented Dec 13, 2017

alshakero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warpech Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warpech Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalec commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warpech Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

warpech commented Dec 13, 2017

tomalec commented Dec 13, 2017

Choose a reason for hiding this comment

tomalec commented Dec 14, 2017

warpech left a comment

Choose a reason for hiding this comment

warpech commented Jan 9, 2018

warpech Dec 13, 2017 •

edited

Loading

warpech Dec 13, 2017 •

edited

Loading

warpech Dec 13, 2017 •

edited

Loading