Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix memory leak in dynamic nested rule #1563

Merged
merged 16 commits into from
Oct 16, 2021
Merged

fix memory leak in dynamic nested rule #1563

merged 16 commits into from
Oct 16, 2021

Conversation

seiyab
Copy link
Member

@seiyab seiyab commented Oct 10, 2021

Corresponding Issue(s):

#1360

What Would You Like to Add/Fix?

Fix memory leak in nested rule in function rule

Todo

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

Expectations on Changes

Resolve memory leak nested rule in function rule

// e.g.
{
  a: ({ color }) => {
    '& button': {
      color
    }
  }
}

Changelog

Fix memory leak with dynamic nested style.

Note

  • For nested-conditional style, I have not changed.
    • Because change might be too big, and it might hold another problem.
    • I would tackle this on another PR.
  • The test for memory leak is 24871d1 . Cherry-picking, you can verify that it works.
  • I added some tests that are not irrelevant to memory leak to be sure that it doesn't break existing features.

@seiyab seiyab requested a review from kof as a code owner October 10, 2021 02:04
seiyab
seiyab commented Oct 10, 2021
View reviewed changes
packages/jss/src/RuleList.js Outdated
Comment on lines 20 to 22
// Rules registry for access by .get() method.
// It contains the same rule registered by name and by selector.
map = {}
nameMap = {}

selectorMap = {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I splitted map to avoid accidental replace via selector.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have a test that shows how it was possible before to accidentally replace by a selector? I don't quite understand when this is the case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was the case.
https://github.com/cssinjs/jss/pull/1563/files#diff-97551384ad757770e360d597385f79789efdd71d55352d2f8b17e0c3ad7f2ec7R131-R157
I fixed name like following and it now passes. So I reverted the splitting.
https://github.com/cssinjs/jss/pull/1563/files#diff-59742e48378650fbbebc47efbcbb17001e1b0926fb6e44742d23969c2973152aL91-R91
To change name, we needed to fix following.
https://github.com/cssinjs/jss/pull/1563/files#diff-e2d69de7d5a671488a4eb6dfe6341d04942a698f362efe143b20008d84908c1aL149-R159

@kof
Copy link
Member

kof commented Oct 10, 2021

Wow this looks great!

I am going to investigate every change to see if everything is correct. The only thing that I don't like so far is the increase in bundle size. I have to think if we can achieve the same thing without adding that much logic.

@seiyab
Copy link
Member Author

seiyab commented Oct 11, 2021

Thank you for your comment.
I reduced bundle size a bit in 50b7b9b .
I removed upsertRule and current replaceRule works like upsert.

kof
kof reviewed Oct 11, 2021
View reviewed changes
packages/jss/src/RuleList.js Outdated Show resolved Hide resolved
kof
kof reviewed Oct 11, 2021
View reviewed changes
packages/jss/src/RuleList.js Outdated Show resolved Hide resolved
kof
kof reviewed Oct 11, 2021
View reviewed changes
packages/jss/src/StyleSheet.js Outdated Show resolved Hide resolved
kof
kof reviewed Oct 11, 2021
View reviewed changes
packages/jss/src/StyleSheet.js Outdated Show resolved Hide resolved
kof
kof reviewed Oct 11, 2021
View reviewed changes
packages/jss/src/StyleSheet.js
// It will be inserted all together when .attach is called.
if (this.renderer) {
if (!newRule) {
this.renderer.deleteRule(oldRule)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why do we delete an oldRule if a new rule was not created?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only case newRule doesn't exist is when user tried to create an unexisting rule type, like some typo or something.

I am not sure but maybe in this case an expected behavior would be to not remove the old rule .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure but maybe in this case an expected behavior would be to not remove the old rule.

I see.
I intended that "when user replace rule by invalid rule, the old rule will replaced by 'nothing'. it results deleting the rule".
Is it better to warn it without rendering effect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I think you are correct, returning null in a function is also removing the rule from dom

kof
kof reviewed Oct 11, 2021
View reviewed changes
packages/jss/src/StyleSheet.js
/**
* Replace a rule in the current stylesheet.
*/
replaceRule(name, decl, options) {
Copy link
Member

@kof kof Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please correct me if I am wrong, but it seems this entire method could be replaced by something this (I haven't tried this to run yet):

const oldRule = this.getRule(name)
const oldRuleIndex = this.indexOf(oldRule)
if (oldRuleIndex !== -1) options = {...options, index: oldRuleIndex}
this.addRule(name, decl, options)
if (oldRule) this.deleteRule(oldRule)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will unregister new rule from RuleList.
I'll try it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment might be wrong.
addRule will generate new name so that we will be able to get new rule by name.
get by selector might not be work.
I'll try registering new rule one more time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests showed some problems.

  1. new rule is registered as name different from the argument
    • if name is a, new rule are registered as a-d0.
  2. replacing rule multiple times, it will render more and more rules
    • consider we use a as a name
    • first replaceRule will register rule as a
    • second replaceRule will register rule as a-d0 and remove a
    • third replaceRule will register rule as a
    • forth replaceRule will register rule as a-d1 and remove a
    • fifth replaceRule will register rule as a
    • now, we have a-d0, a-1 and a

@seiyab
Copy link
Member Author

seiyab commented Oct 11, 2021

Thank you for your careful review 👍
I'll reply or fix each comment.

seiyab and others added 4 commits October 11, 2021 21:24
@seiyab @kof
make test description more meaningful
6cecf32 
Co-authored-by: Oleg Isonen <oleg008@gmail.com>
@seiyab @kof
make test description more meaningful
12b169e 
Co-authored-by: Oleg Isonen <oleg008@gmail.com>
@seiyab
make test descriptions clear
c77b53d
@seiyab
update test decription
801bb95
seiyab added 4 commits October 12, 2021 20:34
@seiyab
prevent unnecessary copy
8a59134
@seiyab
remove RuleList.getByName, StyleSheet.getByName
36839a6 
include related change
- use almost unique name rather than selector name in nested plugin to prevent accidental replacement
- fix strange behavior of global plugin (probably bug)
@seiyab
make replaceRule to return just a newRule
b998b8b
@seiyab
add tests for global container rule
f79c62f
@seiyab
Copy link
Member Author

seiyab commented Oct 12, 2021

For each comment, I fixed or replied.

@seiyab seiyab requested a review from kof October 14, 2021 10:44
@kof kof merged commit 87952b4 into cssinjs:master Oct 16, 2021
@kof
Copy link
Member

kof commented Oct 16, 2021

merged, thanks a lot, this is a huge help!

@kof kof mentioned this pull request Oct 16, 2021
Closed
kof added a commit that referenced this pull request Oct 16, 2021
@kof
changelog for #1563
6b34596
@kof
Copy link
Member

kof commented Oct 16, 2021

released in https://github.com/cssinjs/jss/releases/tag/v10.8.1

@seiyab seiyab mentioned this pull request Oct 17, 2021
Open
kof added a commit that referenced this pull request Oct 25, 2021
@kof
reverting #1563 changelog
c548520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kof kof Awaiting requested review from kof kof is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@seiyab @kof