-
-
Notifications
You must be signed in to change notification settings - Fork 389
fix memory leak again #1574
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.
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
fix memory leak again #1574
Changes from 9 commits
46a0ccd
9b1026f
449b374
0f92bdd
d2d6e14
bc28f31
fb88fa1
d572e3c
6673938
a9dcddf
b1edcce
629865a
630f149
ef59208
57de9d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,8 @@ | |
| "check-snapshot": "node ../../scripts/match-snapshot.js" | ||
| }, | ||
| "devDependencies": { | ||
| "jss-plugin-nested": "10.8.2" | ||
| "jss-plugin-nested": "10.8.2", | ||
| "jss-v10_8_0": "npm:jss@10.8.0" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks strange
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does strange mean?
Removing tests with older jss is OK with me.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I never seen npm: prefix in dependencies and naming with underscore jss-v10_8_0 like this, |
||
| }, | ||
| "dependencies": { | ||
| "@babel/runtime": "^7.3.1", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import {stripIndent} from 'common-tags' | ||
| import expect from 'expect.js' | ||
| import {create} from 'jss' | ||
| import {create as oldCreate} from 'jss-v10_8_0' | ||
| import nested from 'jss-plugin-nested' | ||
|
|
||
| import global from './index' | ||
|
|
@@ -41,6 +42,94 @@ describe('jss-plugin-global', () => { | |
| it('should remove whitespaces', () => { | ||
| expect(sheet.toString({format: false})).to.be('a{color:red;}body{color:green;}') | ||
| }) | ||
|
|
||
| describe('addRule', () => { | ||
| let globalRule | ||
| beforeEach(() => { | ||
| globalRule = sheet.getRule('@global') | ||
| globalRule.addRule('button', {margin: 0}) | ||
| globalRule.addRule('li', {float: 'left'}) | ||
| }) | ||
|
|
||
| it('should add rule', () => { | ||
| expect(globalRule.getRule('button')).to.not.be(undefined) | ||
| expect(globalRule.getRule('li')).to.not.be(undefined) | ||
| }) | ||
|
|
||
| it('should generate correct CSS', () => { | ||
| expect(sheet.toString()).to.be(stripIndent` | ||
| a { | ||
| color: red; | ||
| } | ||
| body { | ||
| color: green; | ||
| } | ||
| button { | ||
| margin: 0; | ||
| } | ||
| li { | ||
| float: left; | ||
| } | ||
| `) | ||
| }) | ||
| }) | ||
|
|
||
| describe('replaceRule', () => { | ||
| let globalRule | ||
| let previousA | ||
| beforeEach(() => { | ||
| globalRule = sheet.getRule('@global') | ||
| previousA = globalRule.getRule('a') | ||
| globalRule.replaceRule('a', {color: 'yellow'}) | ||
| globalRule.replaceRule('li', {float: 'left'}) | ||
| }) | ||
|
|
||
| it('should replace and add rule', () => { | ||
| expect(globalRule.getRule('a')).to.not.be(previousA) | ||
| expect(globalRule.getRule('li')).to.not.be(undefined) | ||
| }) | ||
|
|
||
| it('should generate correct CSS', () => { | ||
| expect(sheet.toString()).to.be(stripIndent` | ||
| a { | ||
| color: yellow; | ||
| } | ||
| body { | ||
| color: green; | ||
| } | ||
| li { | ||
| float: left; | ||
| } | ||
| `) | ||
| }) | ||
| }) | ||
|
|
||
| describe('addRule / replaceRule with selector', () => { | ||
| let globalRule | ||
| beforeEach(() => { | ||
| globalRule = sheet.getRule('@global') | ||
| globalRule.addRule('arbitrary-name-1', {color: 'red'}, {selector: 'span'}) | ||
| globalRule.replaceRule('arbitrary-name-2', {float: 'left'}, {selector: 'ul'}) | ||
| globalRule.replaceRule('a', {display: 'block'}, {selector: 'div'}) | ||
| }) | ||
|
|
||
| it('should generate correct CSS', () => { | ||
| expect(sheet.toString()).to.be(stripIndent` | ||
| div { | ||
| display: block; | ||
| } | ||
| body { | ||
| color: green; | ||
| } | ||
| span { | ||
| color: red; | ||
| } | ||
| ul { | ||
| float: left; | ||
| } | ||
| `) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe('@global linked', () => { | ||
|
|
@@ -420,4 +509,33 @@ describe('jss-plugin-global', () => { | |
| ) | ||
| }) | ||
| }) | ||
|
|
||
| describe('backward compatibility', () => { | ||
| let oldJss | ||
|
|
||
| beforeEach(() => { | ||
| oldJss = oldCreate(settings).use(global()) | ||
| }) | ||
|
|
||
| it('should replace rule on replaceRule even if jss core doesn\'t implement replaceRule', () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we could throw an error if replaceRule doesn't exist and say versions are incompatible, to communicate to the user the problem clearly
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. |
||
| // this is because RuleList that plugin uses has same version with plugin according to dependency | ||
| // even if user accidentally use newer plugin with older jss-core | ||
|
|
||
| const sheet = oldJss.createStyleSheet({ | ||
| '@global': { | ||
| a: {color: 'red'}, | ||
| body: {top: '0px'}, | ||
| } | ||
| }) | ||
| sheet.getRule('@global').replaceRule('a', {color: 'blue'}) | ||
| expect(sheet.toString()).to.be(stripIndent` | ||
| a { | ||
| color: blue; | ||
| } | ||
| body { | ||
| top: 0px; | ||
| } | ||
| `) | ||
| }) | ||
| }) | ||
| }) | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 .
arrowParensis"always"by default in Prettier v2.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets upgrade prettier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upgraded 👍