Rewrite project to make it smaller, faster and more understandable #72

Lodin · 2021-04-29T17:42:04Z

This PR introduces a new approach for initializing and supporting the Constructible Style Sheets. It aims to make code smaller, faster and more clear to understand.

Approach

Now we have two logic entrypoints associated with the specific class:

The CSSStyleSheet implementation (associated with ConstructedStyleSheet class).
The adoptedStyleSheets property (associated with Location class).

Details

The iframe element is not used anymore. Instead, the in-memory HTML document created by document.implementation.createHTMLDocument() is used. Thanks to that improvement, we no longer need to rely on the document loading status; in-memory document can be created even in <head>.
Following the performance idea, all CSS rules (including ones added by replace and replaceSync) are done via CSSStyleSheet.insertRule.
Each object that can have the adoptedStyleSheets property (Document and ShadowRoot) is wrapped with the associated Location class instance that watches internal DOM tree and connects (initialzies) each added element; it also disconnects each removed element.
ConstructedStyleSheet no more reveals the internal stylesheet; instead, it imitates the CSSStyleSheet behavior.
The adoptedStyleSheets property works closer to specification and doesn't allow adding non-constructable style sheet.
For better type-safety, the Typescript support added for both core and tests.
Code is written in ES5 to avoid including Babel helpers and reduce the general size of the polyfill.
Private properties are emulated via WeakMap. For ConstructedStyleSheet it allows avoiding access to internal properties by the end user; for Location it reduces the size of the polyfill when minified.
Added commands for Terser to inline some functions; it also allows reducing the size of the build.
The husky package is replaced with simple-git-hooks due to license and performance issues.
All dependencies are updated to the latest versions.
IE11 regression fixed.
Added size-limit package for checking the project size with limit of 2 kB.
@rollup/plugin-typescript is used only because babel does not support declare class A {} along with function A() {} which is necessary for correct typings of internal classes. @rollup/plugin-typescript only removes types. For tests, to add coverage @rollup/plugin-babel is used.

Size

Old package size (by size-limit): 2.12 kB.
New package size (by size-limit): 1.82 kB.

Special thanks

Special thanks to @platosha for suggesting the idea of document.implementation.createHTMLDocument().

Breaking changes?

In theory, it shouldn't affect the users because both versions comply the specification of Constructible Style Sheets. Howerver, I would suggest to release it as beta first to get the initial user feedback.

I would also suggest to merge #34 first. With it, we will be able to run tests in all browsers including Safari and IE11.

* Finish fixing tests for IE11 * Move code to the src directory

codecov-commenter · 2021-04-29T18:15:19Z

Codecov Report

Merging #72 (1ae8460) into master (b793dd7) will increase coverage by 90.86%.
The diff coverage is 90.86%.

@@             Coverage Diff             @@
##           master      #72       +/-   ##
===========================================
+ Coverage        0   90.86%   +90.86%     
===========================================
  Files           0        5        +5     
  Lines           0      186      +186     
  Branches        0       33       +33     
===========================================
+ Hits            0      169      +169     
- Misses          0        4        +4     
- Partials        0       13       +13

Impacted Files	Coverage Δ
src/shared.ts	`50.00% <50.00%> (ø)`
src/index.ts	`80.00% <80.00%> (ø)`
src/Location.ts	`91.95% <91.95%> (ø)`
src/ConstructedStyleSheet.ts	`93.75% <93.75%> (ø)`
src/utils.ts	`93.75% <93.75%> (ø)`

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 b793dd7...1ae8460. Read the comment docs.

calebdwilliams · 2021-04-30T12:42:49Z

Nice, I’ve actually been meaning to make a ton of these changes but haven’t had the time.

I definitely think this constitutes a breaking change so well increment the major version before releasing. There’s a lot here, so it might take me a few days to put it through it’s paces, but my first skim looked good. Not entirely sure about all ES5, but I’ll let it sink in.

calebdwilliams

Just a cursory look at this. I'll dive in deeper a bit later.

calebdwilliams · 2021-05-01T01:14:54Z

package.json

    "typecheck": "tsc --noEmit"
  },
  "repository": {
    "type": "git",
    "url": "git+https://github.com/calebdwilliams/adoptedStyleSheets.git"
  },
-  "author": "Caleb D. Williams <[email protected]>",
+  "authors": [


I don't technically think this is valid in package.json. Can you revert but add a contributors? You certainly deserve credit and I'm more than happy to give it where it's due; really appreciate the effort here.

Oh right. I replaced it with contributors instead.

calebdwilliams · 2021-05-01T01:31:11Z

src/ConstructedStyleSheet.ts

+    clearRules(basic);
+
+    if (sanitized) {
+      basic.insertRule(sanitized, 0);


Unless I'm missing something this will fail if there is more than one rule in the stylesheet. Changing the test to

headingStyles .replace( ` h1 { color: tomato; } body { background: mediumaquamarine; }`, ) .then(sheet => { console.log(`${sheet} styles have been replaced.`); console.assert(sheet === headingStyles, 'not the same', 'yass'); });

will throw, hence the original innerHTML call. We could potentially parse the thing with a regexp and see if insertRule is faster, but I doubt it.

Good catch, thanks! I replaced it with the basic.ownerNode.textContent = sanitized. It will do all the parsing job for us and still allow using restyleAdopter with insertRule approach.

src/ConstructedStyleSheet.ts

src/Location.ts

calebdwilliams · 2021-05-01T01:48:28Z

src/Location.ts

+  removedSheets.forEach(function (sheet) {
+    // Type Note: any removed sheet is already initialized, so there cannot be
+    // missing adopter for this location.
+    /*#__INLINE__*/ removeNode(getAdopterByLocation(sheet, self)!);


Are these /*#__INLINE__*/ comments necessary?

They are not but it would allow Terser to understand that these functions can be inlined. I don't want to do that manually because of readability but it would be nice if we give user a chance to reduce some bytes during the minification.

Just checked: you're right, they are unnecessary; Terser does inlining based on amount of function calls. So, I removed them.

src/index.ts

calebdwilliams · 2021-05-01T01:54:09Z

src/utils.ts

+}
+
+/*#__PURE__*/
+export function insertAllRules(from: CSSStyleSheet, to: CSSStyleSheet): void {


Probably want to use this above instead of this.insertRule. I still wonder about the performance about these two and if it might cause unwanted flash of styles. Should this be in a requestAnimationFrame? Again, I'm mostly just thinking out loud. I will test these either this weekend or early next week.

I tried both current and requestAnimationFrame approaches and don't see any specific difference in visualization and the flame chart. Could you test it as well? I think I would add requestAnimationFrame call anyway just to be sure it works at correct time.

calebdwilliams · 2021-05-01T01:59:21Z

@Lodin Also, I really think modern syntax like let/const and some (though not all) arrow functions could help the readability here. We can set the TS target to ES5 with no problems here to utilize those things. If nothing else it provides information to the developer what the intent of the variables are and TypeScript will compile those away nicely.

Lodin · 2021-05-07T10:19:45Z

Also, I really think modern syntax like let/const and some (though not all) arrow functions could help the readability here.

Well, I'm ok with it. Just thought that if we try to avoid any compilation artifacts it may be better to use ES5 approach as much as possible. However, I agree with your arguments.

* Fix issue with multiple rules replacement. * Use let/const and arrow functions wherever possible. * Use "contributors" in package.json

calebdwilliams · 2021-05-14T16:30:17Z

@Lodin, let me know when you're happy with this PR and I'll do a final review.

Lodin · 2021-05-14T17:08:45Z

@calebdwilliams, it's ready, actually. I'm not going to do further changes before your review.

calebdwilliams · 2021-06-16T12:55:20Z

@Lodin, I'm ready to merge/publish this, but I would like to do it as a major version/prerelease. In order to do that, I'd prefer to keep it in a different branch for a bit first. Would you mind setting that up so I can merge/release? Great work here, thanks for all your contributions to this project!

Lodin · 2021-06-16T15:16:48Z

@calebdwilliams, great news! What the branch would you like to use then? Also, what the version should it have?

calebdwilliams · 2021-06-16T15:59:32Z

I’d keep it simple like 3.0-rc or something like that (I’m not picky). I’ll take care of the version number when I publish it.

Lodin · 2021-06-17T06:19:15Z

Looks like I can re-direct the PR only if the branch exists. Could you please create a 3.0-rc branch then?

calebdwilliams · 2021-06-17T15:58:55Z

@Lodin done

Lodin · 2021-06-18T06:02:12Z

@calebdwilliams, I have re-directed the PR to the 3.0-rc branch

calebdwilliams · 2021-06-18T19:27:07Z

@Lodin this has been released as [email protected].

Lodin added 11 commits April 18, 2021 00:07

refactor: use simpler approach

12ef036

refactor: update tests and fix code accordingly

0cba558

refactor: finalize code & tests

1a44cc3

* Finish fixing tests for IE11 * Move code to the src directory

refactor: use forEach for array-like structures

17be7ea

refactor: convert to Typescript & add more documentation

8da9d19

refactor: add small fixes and tweaks for Terser

c0423bd

test: update tests

bb165f0

chore: update dependencies and building pipeline

ffefb46

chore: replace husky with simple-git-hooks

da50b17

chore: add size-limit check

3394b0f

chore: add tslib package required by @rollup/plugin-typescript

14cc50b

calebdwilliams requested changes May 1, 2021

View reviewed changes

Lodin added 3 commits May 7, 2021 14:46

refactor: address review comments

e28c533

* Fix issue with multiple rules replacement. * Use let/const and arrow functions wherever possible. * Use "contributors" in package.json

refactor: use more self instead of this

dab4fc7

test: add tests for multi-rule style replacement

1681d7b

Lodin requested a review from calebdwilliams May 7, 2021 13:51

calebdwilliams mentioned this pull request May 14, 2021

Other micro tasks can be executed while initPolyfill is running #76

Closed

refactor: remove Terser annotations

1ae8460

Lodin changed the base branch from master to 3.0-rc June 18, 2021 06:00

calebdwilliams merged commit ad64cf5 into calebdwilliams:3.0-rc Jun 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite project to make it smaller, faster and more understandable #72

Rewrite project to make it smaller, faster and more understandable #72

Lodin commented Apr 29, 2021 •

edited

Loading

codecov-commenter commented Apr 29, 2021 •

edited

Loading

calebdwilliams commented Apr 30, 2021

calebdwilliams left a comment

calebdwilliams May 1, 2021

Lodin May 7, 2021 •

edited

Loading

calebdwilliams May 1, 2021

Lodin May 7, 2021

calebdwilliams May 1, 2021

Lodin May 7, 2021

Lodin May 14, 2021

calebdwilliams May 1, 2021

Lodin May 7, 2021

calebdwilliams commented May 1, 2021

Lodin commented May 7, 2021 •

edited

Loading

calebdwilliams commented May 14, 2021

Lodin commented May 14, 2021

calebdwilliams commented Jun 16, 2021

Lodin commented Jun 16, 2021

calebdwilliams commented Jun 16, 2021

Lodin commented Jun 17, 2021

calebdwilliams commented Jun 17, 2021

Lodin commented Jun 18, 2021

calebdwilliams commented Jun 18, 2021

Rewrite project to make it smaller, faster and more understandable #72

Rewrite project to make it smaller, faster and more understandable #72

Conversation

Lodin commented Apr 29, 2021 • edited Loading

Approach

Details

Size

Special thanks

Breaking changes?

codecov-commenter commented Apr 29, 2021 • edited Loading

Codecov Report

calebdwilliams commented Apr 30, 2021

calebdwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lodin May 7, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebdwilliams commented May 1, 2021

Lodin commented May 7, 2021 • edited Loading

calebdwilliams commented May 14, 2021

Lodin commented May 14, 2021

calebdwilliams commented Jun 16, 2021

Lodin commented Jun 16, 2021

calebdwilliams commented Jun 16, 2021

Lodin commented Jun 17, 2021

calebdwilliams commented Jun 17, 2021

Lodin commented Jun 18, 2021

calebdwilliams commented Jun 18, 2021

Lodin commented Apr 29, 2021 •

edited

Loading

codecov-commenter commented Apr 29, 2021 •

edited

Loading

Lodin May 7, 2021 •

edited

Loading

Lodin commented May 7, 2021 •

edited

Loading