-
-
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 13 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 |
|---|---|---|
| @@ -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; | ||
| } | ||
| `) | ||
| }) | ||
| }) | ||
| }) | ||
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.
this looks strange
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.
What does strange mean?
Removing tests with older jss is OK with me.
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 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