fix memory leak again by seiyab · Pull Request #1574 · cssinjs/jss

seiyab · 2021-10-29T13:40:44Z

Corresponding Issue(s):

#1360 #1565

What Would You Like to Add/Fix?

Memory leak in nested function rule

Todo

Add test(s) that verify the modified behavior
Add documentation if it changes public API

Expectations on Changes

Changelog

This reverts commit 45f4463. try to fix memory leak again

- not assert CSSOM - assert entier styles after update

kof · 2021-10-29T18:50:10Z

+    let style
+    let sheet
+
+    // We skip this test as keyframes are not supported by browser.


we should drop support the support of IE9, probably even IE10 too, so this won't be needed, but also not in the same pr, just letting you know

kof · 2021-10-29T18:51:28Z

Good work @seiyab, which change fixes the concerns we had with the previous PR? I mean besides of splicing the index

seiyab · 2021-10-30T02:01:20Z

@kof
Current work just addresses splicing the index.
I'll work with replaceRule is not a function in other commits in this PR. mui/mui-x#2993 (comment)
And I'm going to add more tests.

Co-authored-by: Oleg Isonen <oleg008@gmail.com>

seiyab · 2021-11-07T14:09:09Z

Current progress

fixed
- splicing cssRules index
- replaceRule is not a function
working
- react-jss might still cause memory leak. investigating.

kof · 2021-11-07T21:54:52Z

just in case - react-css isn't documented, its an experimental interface that hopefully nobody is using in production

seiyab · 2021-11-08T01:08:50Z

Oh I mistyped. I edited my comment. react-css → react-jss

seiyab · 2021-11-08T11:59:25Z

The memory leak I encountered was generated by nested conditional rule, that is out of scope for this fix.
This PR might be ready for review. I will check changes I wrote before pushing "Ready for review" button.

seiyab · 2021-11-08T12:41:37Z

  "bracketSpacing": false,
-  "printWidth": 100
+  "printWidth": 100,
+  "arrowParens": "always"


I saw you added parentheses, that is removed by Prettier that runs on pre-commit.
So I changed this configuration to prevent removing parentheses from Prettier.
c60f395

I found lint you committed is more likely written by Prettier v2 rather than v1 installed by devDependency.
When I accidentally run prettier v2 installed globally in my machine and the result was similar to c60f395 .
arrowParens is "always" by default in Prettier v2.

lets upgrade prettier

upgraded 👍

kof · 2021-11-12T17:32:48Z

  "devDependencies": {
-    "jss-plugin-nested": "10.8.2"
+    "jss-plugin-nested": "10.8.2",
+    "jss-v10_8_0": "npm:jss@10.8.0"


this looks strange

What does strange mean?

Doesn't work well?

Not good idea?

Removing tests with older jss is OK with me.

I never seen npm: prefix in dependencies and naming with underscore jss-v10_8_0 like this,
but yeah, lets figure out if we really need to have this compatibility logic and tests

kof · 2021-11-12T17:34:54Z

+      oldJss = oldCreate(settings).use(global())
+    })
+
+    it('should replace rule on replaceRule even if jss core doesn\'t implement replaceRule', () => { 


why do we need this test? react-jss comes bundled with jss core, so its there, if somebody is using an incompatible version - its on them

we should communicate incompatibility in the changelog, make sure what we expect to work - works, maintaining compatibility logic and compatibility tests feels like a lot of future work

maybe we could throw an error if replaceRule doesn't exist and say versions are incompatible, to communicate to the user the problem clearly

That makes sense.
I removed tests that use incompatible version.

kof · 2021-11-14T09:59:04Z

  "devDependencies": {
-    "jss-plugin-nested": "10.8.2"
+    "jss-plugin-nested": "10.8.2",
+    "jss-v10_8_0": "npm:jss@10.8.0"


I never seen npm: prefix in dependencies and naming with underscore jss-v10_8_0 like this,
but yeah, lets figure out if we really need to have this compatibility logic and tests

kof · 2021-11-23T03:46:59Z

Just realized I never sent the last review, it was in the pending state

kof · 2021-12-08T10:14:13Z

merged

kof · 2021-12-08T10:26:24Z

released in v10.9.0

seiyab added 5 commits October 29, 2021 21:38

Revert "Revert 87952b4 because of cssinjs#1565"

46a0ccd

This reverts commit 45f4463. try to fix memory leak again

change arrowParens in .prettierrc

9b1026f

fix unintended replace in DOM

449b374

add test for replaceRule in sheet with many rules

0f92bdd

modify test in react-jss

d2d6e14

- not assert CSSOM - assert entier styles after update

seiyab requested a review from kof as a code owner October 29, 2021 13:40

kof reviewed Oct 29, 2021

View reviewed changes

Comment thread packages/jss/tests/functional/sheet.js Outdated

kof reviewed Oct 29, 2021

View reviewed changes

seiyab and others added 4 commits October 30, 2021 11:04

Update test description

bc28f31

Co-authored-by: Oleg Isonen <oleg008@gmail.com>

fix backward compatibility problem in nested rule

fb88fa1

backward compatibility tests

d572e3c

fix type mismatch

6673938

seiyab marked this pull request as draft November 4, 2021 10:32

seiyab closed this Nov 8, 2021

seiyab reopened this Nov 8, 2021

run prettier

a9dcddf

seiyab commented Nov 8, 2021

View reviewed changes

remove CSSOM based test in plugin-rule-vaue-function

b1edcce

seiyab marked this pull request as ready for review November 8, 2021 13:35

seiyab requested a review from kof November 8, 2021 13:35

seiyab changed the title ~~WIP: fix memory leak again~~ fix memory leak again Nov 8, 2021

seiyab added 2 commits November 10, 2021 00:05

upgrade prettier

629865a

run prettier v2

630f149

kof reviewed Nov 12, 2021

View reviewed changes

v10.8.3

ef59208

kof reviewed Nov 23, 2021

View reviewed changes

remove tests with incompatible dependency

57de9d0

seiyab requested a review from kof November 24, 2021 11:37

cssinjs deleted a comment from oleg008 Dec 8, 2021

kof closed this Dec 8, 2021

seiyab deleted the 1360-fix-memory-leak-again branch December 8, 2021 12:15

Uh oh!

Conversation

seiyab commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Corresponding Issue(s):

What Would You Like to Add/Fix?

Todo

Expectations on Changes

Changelog

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kof commented Oct 29, 2021

Uh oh!

seiyab commented Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seiyab commented Nov 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kof commented Nov 7, 2021

Uh oh!

seiyab commented Nov 8, 2021

Uh oh!

seiyab commented Nov 8, 2021

Uh oh!

seiyab Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kof commented Nov 23, 2021

Uh oh!

kof commented Dec 8, 2021

Uh oh!

kof commented Dec 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

seiyab commented Oct 29, 2021 •

edited

Loading

seiyab commented Oct 30, 2021 •

edited

Loading

seiyab commented Nov 7, 2021 •

edited

Loading

seiyab Nov 8, 2021 •

edited

Loading