Make the polyfill work in IE & Edge #28

Lodin · 2019-08-31T13:54:44Z

Resolves #12.
Resolves #25.
Fixes #29.

This PR provides support for IE in the polyfill.

@calebdwilliams, sorry for jumping in, I know that you're working on the IE support but you mentioned that you don't have IE11 machine now. So I decided to start working on it in parallel since I have both IE11 and Edge on my machine. Please, feel free to add any solution you prefer.

PR is still in the early stage, a lot of tests are red in IE and Edge now. It may take some time to find a fix all of them.

calebdwilliams · 2019-08-31T16:40:26Z

I think this makes sense if you have immediate access to IE11. As for polyfills, IE supports WeakMap and Map (but not iterable Maps). I would prefer using Rollup to bundle here (or just npm scripts) because the output will be cleaner.

calebdwilliams · 2019-08-31T16:44:15Z

I did also want to say this package should not include its own polyfills for Promise or Map to ensure compatibility with other tools that might be used. Instead please add notes inside the README that note what will be needed.

For example, if I'm running an Angular application, I want the Promise used in this polyfill to be the same one loaded by Angular (or any other framework). I can still be convinced on the Node.isConnected polyfill, but that has less impact on consuming projects.

Lodin · 2019-08-31T18:00:05Z

As for polyfills, IE supports WeakMap and Map (but not iterable Maps).

Oh, does it? I thought that it is a ES6 thing for some reason. That's the good news.

I would prefer using Rollup to bundle here (or just npm scripts) because the output will be cleaner.

You mean to transpile the existing code? Well, we can do that but, AFAIK, all polyfills are written with ES5 to make manual tweaks (I did them in the code as well) and get the resulting code size smaller. I personally would prefer to follow this tradition. What do you think about it?

I did also want to say this package should not include its own polyfills for Promise or Map to ensure compatibility with other tools that might be used.

Sure, I completely agree here. Polyfills should not be dependencies, they are the support code that the end user adds if necessary.

I can still be convinced on the Node.isConnected

BTW, I think I found a workaround to not use the polyfill at all. The idea is that if we have ShadowRoot available (I mean, the native ShadowRoot), the browser should definitely support isConnected. And if not, we can just run the simple document.contains check.

Lodin · 2019-08-31T18:58:51Z

@calebdwilliams, are you aware how to work with the ShadyCSS? I'm kinda stuck with it. It has its own implementation for adoptedStyleSheets, so all we need to do if it is defined in window is to rely on its API. However, I cannot find any appropriate docs or tutorial on how to use this API. Do you know something?

calebdwilliams · 2019-08-31T19:32:25Z

Really good thought on ShadowRoot and Node.isConnected. I like that approach a lot.

Not sure what you mean here:

You mean to transpile the existing code? Well, we can do that but, AFAIK, all polyfills are written with ES5 to make manual tweaks (I did them in the code as well) and get the resulting code size smaller. I personally would prefer to follow this tradition. What do you think about it?

Lodin · 2019-08-31T19:40:36Z

@calebdwilliams, hmm, I'm about your mention of the Rollup. I might mistunderstood you; what did you mean regarding Rollup?

calebdwilliams · 2019-09-01T13:35:53Z

So thinking about this a bit, I believe it would be better to keep the src file in ES6+ and Babel down to IE11 support (primarily to keep the code as maintainable as possible). My ideal setup would use Rollup and split out the source into src and maybe divide the code a bit. Then use Babel to output the file in the project’s root. I realize you have done a lot of work on this already, let’s pause though and consider what approach is s most sustainable long term.

Lodin · 2019-09-01T14:14:01Z

Well, I don't think that keeping the code in ES5 would be harder than work with it in ES6. I rewrote it, and I don't see much difference. The most visible changes are the following:

const/let -> var. Well, that's a mostly cosmetic change.
for..of -> forEach. This change should be done in both variants to support IE11 native Map forEach.
No destructuring. That's the most painful, probably, but I don't see it as a show-stopper. Now it is wordier, but still quite understandable.
No arrow functions. It brings some troubles with this but it is not very popular in the code, so I think I've already fixed every issue.

So, I don't see a lot of disadvantages while there is some advantages:

No building system, no additional configuration, the file can be used in any browser with any loading system.
While we work with ES5, we understand that there could be some caveats like this one. In ES6 it would be surprising because ES6 is supposed to work with polyfills to fix this type of issues (well, at least I think this way).
We can optimize some ES5 code better than Babel would do it. E.g., here I just use the arguments object when Babel inserts for loop to rewrite arguments to an args array. Well, that's not a lot of work nor the bytes, but with the cases above it may play its role.

Plus, as I mentioned above, the polyfills are primarily written in ES5 to be consumed by old browsers directly. I believe we should follow this tradition.

That are my ideas regarding this question. I do not totally disagree with ES6 solution, but while debugging the code in Edge and IE11, I found that it would be way easier to work with ES5 code than a transpiled ES6 (especially it is connected with async-await). That was the reason to rewrite tests to ES5 as well.

Lodin · 2019-09-01T17:40:08Z

Ok, from the #30 it looks like that we would need more files to properly support CSSStylesSheet rules. So I agree with ES6 build and Rollup.

Lodin · 2019-09-01T23:19:55Z

Hmm, I would say that I'm kinda stuck with the ShadyCSS. I'm not sure it can be properly integrated; all my attempts are like making the polyfill more complicated than it should be.

I'm going to push all changes in the current state, then purge all ShadyCSS-related things and then make PR available. For IE it will work only for the document but not for polyfilled custom elements. Tests will be passed only in Firefox, Chrome, and Safari (or we can disable Custom Element tests for IE & Edge).

So there will be a history of ShadyCSS attempts, and if someone can make it work, it would be great.

Lodin · 2019-09-01T23:49:01Z

By the way, I'm not sure we should try to integrate it since ShadyCSS has its own implementation already. Support of adopted style sheets in shadow roots is tightly coupled with supporting web components, and polyfilling web components is definitely out of scope for this project.

* Split code to separate files * Add Rollup build with detection injection * Attempt to work together with ShadyCSS

codecov-io · 2019-09-02T07:42:41Z

Codecov Report

Merging #28 into master will increase coverage by 0.17%.
The diff coverage is 95.59%.

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
+ Coverage   95.42%   95.59%   +0.17%     
==========================================
  Files           1        7       +6     
  Lines         153      159       +6     
==========================================
+ Hits          146      152       +6     
  Misses          7        7

Impacted Files	Coverage Δ
src/observer.js	`100% <100%> (ø)`
src/utils.js	`100% <100%> (ø)`
src/shared.js	`100% <100%> (ø)`
src/index.js	`83.33% <83.33%> (ø)`
src/init.js	`93.1% <93.1%> (ø)`
src/ConstructStyleSheet.js	`93.47% <93.47%> (ø)`
src/adopt.js	`97.5% <97.5%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c6a1d8...52cc5c7. Read the comment docs.

Lodin · 2019-09-02T07:52:14Z

Hmm, the version of Firefox and Chrome in travis is rather old (56 when the current is 68), it might be a reason why tests fail. I have all of them passing on my local Firefox 68.

calebdwilliams · 2019-09-03T14:45:19Z

Sorry I've been out of the pocket over the weekend, it was a holiday here in the US yesterday, but I'm back online. Can you get some info about why the tests are failing? I'll see if I can't figure out how to upgrade Firefox in Travis (or if that's even possible).

Lodin · 2019-09-03T15:26:24Z

@calebdwilliams, probably, it does not support Web Components. AFAIK, Firefox supports them starting from the version 63 (October of 2018) while Travis suggests version 56.

I really don't understand why it does provide such old browsers. My project runs on the Chrome 76, and my .travis.yml file does not have much difference from yours. The only big difference I can see is that your project runs on https://travis-ci.com (the new one) and my - on the https://travis-ci.org (the old one). But I don't see any particular reason to load such different versions.

However, let's try setting Firefox from stable to latest (I've seen it in the Travis docs)

Lodin · 2019-09-03T15:29:10Z

By the way, what do you think about my suggestion of not supporting ShadyCSS at all since it has its own support? We would be able to support only the document level of adoptedStyleSheet and it may play fine with ShadyCSS implementation of Constructible Style Sheets.

calebdwilliams · 2019-09-03T15:34:24Z

Let's put ShadyCSS support on the back burner for now. That might be something to revisit in the future, but that is tangential to the primary goal of this PR.

As for Firefox, please feel free to update the Travis config. I saw both versions listed on the Travis documentation, so it could have been a mis-type somewhere.

Edit

FWIW, let's remove any attempt to get ShadyCSS working (maybe create a new branch to retain what you currently have so we can revisit later). Before this update is ready, I'd like these commits to be squashed, too.

Lodin · 2019-09-03T16:14:54Z

I already purged all attempts to get ShadyCSS working.

Before this update is ready, I'd like these commits to be squashed, too.

Maybe, it would be better to squash the whole PR? You can do it via the Github interface:

Lodin · 2019-09-03T16:18:26Z

Ok, looks like it works :) Tests are passed

Lodin · 2019-09-03T16:29:36Z

Oh wait, one more thing to remove

Lodin · 2019-09-03T19:04:58Z

Ok, this PR is ready, let's squash it and merge 🙂

calebdwilliams

I haven't had time to give this a thorough look through just yet, but I do have a couple initial questions/feedback.

README.md

karma.conf.js

scripts/build.js

…ecessary code

calebdwilliams · 2019-09-04T14:58:18Z

I was also thinking Element.insertAdjacentElement with 'afterbegin' might be a better solution for our Element.append workaround.

Lodin · 2019-09-04T17:01:46Z

Unfortunately, Element.insertAdjacentElement works only with objects that implement Element interface, not the Node. So, we cannot use it with the DocumentFragment. I don't know why it is designed this way, such a disappointment.

calebdwilliams

Really minor comments. Also add the instanceof language to README so the limitation in IE11 is documented. Once those last few things are ready I'll squash and merge.

calebdwilliams · 2019-09-05T19:33:00Z

test/_index.html

@@ -5,6 +5,6 @@
 <body>
    <h1>Hello world</h1>
 </body>
-<script src="../adoptedStyleSheets.js"></script>
+<script type="module" src="../src/index.js"></script>


Better to load from dist or set up a script to run this from localhost otherwise this doesn't work from file://

Oh yeah, I forgot about this part.

I mean, it's still [type=module] which won't load from file://

calebdwilliams · 2019-09-05T19:33:14Z

test/index.html

@@ -3,12 +3,12 @@
    <head>
        <title>Constructed style sheets test</title>
        <meta charset="UTF-8">
-        <script src="../adoptedStyleSheets.js"></script>
+        <script src="../src/index.js" type="module"></script>


Better to load from dist or set up a script to run this from localhost otherwise this doesn't work from file://

calebdwilliams · 2019-09-05T22:00:55Z

test/_index.html

@@ -5,6 +5,6 @@
 <body>
    <h1>Hello world</h1>
 </body>
-<script src="../adoptedStyleSheets.js"></script>
+<script type="module" src="../src/index.js"></script>


I mean, it's still [type=module] which won't load from file://

calebdwilliams · 2019-09-05T22:01:55Z

README.md

+required.
+
+For browsers that do not support the web components specification (currently
+IE 11 and Edge) only the document-level style sheets adoption works. 


And checking instanceof will not work in IE11 due to Symbol not being natively supported.

Hmm, I am sure that I fixed it and the previous one. It looks the following now:

<script src="../dist/adoptedStyleSheets.js"></script>

and

## Support This polyfill supports all modern browsers and IE 11. For browsers that do not support the web components specification (currently IE 11 and Edge) only the document-level style sheets adoption works. ### IE 11 To make this polyfill work with IE 11 you need the following tools: - `Symbol` polyfill (with support for `Symbol.hasInstance`). - [@babel/plugin-transform-instanceof](https://www.npmjs.com/package/@babel/plugin-transform-instanceof) applied to your code that uses `instanceof` against `CSSStyleSheet`.

…sts are ignored for IE

calebdwilliams

LGTM

pika-ci · 2019-09-06T03:55:38Z

🚀 This PR has been merged! Once a new release is created, any changes will become available on npm. Until then, you can load and install it directly from the Pika CDN:

npm install https://cdn.pika.dev/construct-style-sheets-polyfill/master
import * as pkg from 'https://cdn.pika.dev/construct-style-sheets-polyfill/master';

Lodin added 2 commits August 31, 2019 16:42

refactor: rewrite polyfill to support IE & update configuration

7b806ab

refactor: rewrite tests to ES5

3c31a2b

calebdwilliams mentioned this pull request Aug 31, 2019

Find alternative to isConnected #25

Closed

fix: add various fixes to make IE & ShadyCSS working

1a462f8

Lodin changed the title ~~[WIP] Make the polyfill work in IE~~ [WIP] Make the polyfill work in IE & Edge Aug 31, 2019

Lodin mentioned this pull request Sep 1, 2019

[Suggestion] Remove support for CSSStyleSheet methods #30

Closed

fix: add some more fixes

3d0d28c

Lodin added 3 commits September 2, 2019 10:30

fix: miscellaneous improvements

2895c86

* Split code to separate files * Add Rollup build with detection injection * Attempt to work together with ShadyCSS

fix: drop support for ShadyCSS

209fe3b

chore: remove unnecessary configuration & dependencies

d6da341

Lodin marked this pull request as ready for review September 2, 2019 07:52

Lodin changed the title ~~[WIP] Make the polyfill work in IE & Edge~~ Make the polyfill work in IE & Edge Sep 2, 2019

refactor: improve naming

c9de0c2

chore: attempt to fix Travis version of Firefox (1)

efd65b7

Lodin added 5 commits September 3, 2019 21:27

refactor: fix all IE & Edge tests

2ea2d51

chore(travis): add OS windows and OS X to test on Safari, Edge and IE

9911519

chore(travis): fix version for Safari

686b666

chore(travis): install proper node version before tests

5fb3ef2

chore(travis): revert all attempts

7c77ff6

calebdwilliams reviewed Sep 3, 2019

View reviewed changes

README.md Show resolved Hide resolved

karma.conf.js Outdated Show resolved Hide resolved

scripts/build.js Show resolved Hide resolved

Lodin added 2 commits September 4, 2019 13:03

refactor: use arguments instead of rest args, because it produces unn…

6c5b3a6

…ecessary code

refactor: make some optimizations

977aba2

Lodin added 3 commits September 5, 2019 02:21

refactor: replace webpack with rollup bundle & update docs

1e9ac11

chore: create result file directory if it does not exist

a60daf4

chore: add "files" directive to package.json

3fcacd8

calebdwilliams requested changes Sep 5, 2019

View reviewed changes

Lodin added 2 commits September 6, 2019 00:30

docs: fix support description for IE 11

5c372b5

test: use built polyfill

317e3f7

Lodin requested a review from calebdwilliams September 5, 2019 21:34

calebdwilliams requested changes Sep 5, 2019

View reviewed changes

test: revert web components workaround for IE since web components te…

52cc5c7

…sts are ignored for IE

Lodin requested a review from calebdwilliams September 5, 2019 23:31

calebdwilliams approved these changes Sep 6, 2019

View reviewed changes

calebdwilliams merged commit d47264b into calebdwilliams:master Sep 6, 2019

Make the polyfill work in IE & Edge #28

Make the polyfill work in IE & Edge #28

Conversation

Lodin commented Aug 31, 2019 • edited Loading

calebdwilliams commented Aug 31, 2019

calebdwilliams commented Aug 31, 2019

Lodin commented Aug 31, 2019 • edited Loading

Lodin commented Aug 31, 2019 • edited Loading

calebdwilliams commented Aug 31, 2019

Lodin commented Aug 31, 2019

calebdwilliams commented Sep 1, 2019

Lodin commented Sep 1, 2019

Lodin commented Sep 1, 2019

Lodin commented Sep 1, 2019 • edited Loading

Lodin commented Sep 1, 2019 • edited Loading

codecov-io commented Sep 2, 2019 • edited Loading

Codecov Report

Lodin commented Sep 2, 2019 • edited Loading

calebdwilliams commented Sep 3, 2019

Lodin commented Sep 3, 2019

Lodin commented Sep 3, 2019

calebdwilliams commented Sep 3, 2019 • edited Loading

Edit

Lodin commented Sep 3, 2019

Lodin commented Sep 3, 2019

Lodin commented Sep 3, 2019

Lodin commented Sep 3, 2019

calebdwilliams left a comment

Choose a reason for hiding this comment

calebdwilliams commented Sep 4, 2019 • edited Loading

Lodin commented Sep 4, 2019 • edited Loading

calebdwilliams 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

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

Lodin Sep 5, 2019 • edited Loading

Choose a reason for hiding this comment

calebdwilliams left a comment

Choose a reason for hiding this comment

pika-ci bot commented Sep 6, 2019

Lodin commented Aug 31, 2019 •

edited

Loading

Lodin commented Aug 31, 2019 •

edited

Loading

Lodin commented Aug 31, 2019 •

edited

Loading

Lodin commented Sep 1, 2019 •

edited

Loading

Lodin commented Sep 1, 2019 •

edited

Loading

codecov-io commented Sep 2, 2019 •

edited

Loading

Lodin commented Sep 2, 2019 •

edited

Loading

calebdwilliams commented Sep 3, 2019 •

edited

Loading

calebdwilliams commented Sep 4, 2019 •

edited

Loading

Lodin commented Sep 4, 2019 •

edited

Loading

Lodin Sep 5, 2019 •

edited

Loading