Skip to content

Commit a68131d

Browse files
inletinlet
authored
Merge pull request #292 from adsko/skipped-return
Fix skipped return statement in custom applyProps
2 parents adea51a + b869806 commit a68131d

File tree

9 files changed

+130
-20
lines changed

9 files changed

+130
-20
lines changed

‎src/components/AnimatedSprite.js

+5-1
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,20 @@ const AnimatedSprite = (root, props) => {
1919
animatedSprite.applyProps = (instance, oldProps, newProps) => {
2020
const { textures, isPlaying, initialFrame, ...props } = newProps
2121

22-
applyDefaultProps(instance, oldProps, props)
22+
let changed = applyDefaultProps(instance, oldProps, props)
2323

2424
if (textures && oldProps['textures'] !== textures) {
2525
instance.textures = makeTexture(textures)
26+
changed = true
2627
}
2728

2829
if (isPlaying !== oldProps.isPlaying || initialFrame !== oldProps.initialFrame) {
2930
const frame = typeof initialFrame === 'number' ? initialFrame : animatedSprite.currentFrame || 0
3031
animatedSprite[isPlaying ? 'gotoAndPlay' : 'gotoAndStop'](frame)
32+
changed = true
3133
}
34+
35+
return changed
3236
}
3337

3438
return animatedSprite

‎src/components/Graphics.js

+4-1
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,14 @@ const Graphics = (root, props) => {
55
const g = new PixiGraphics()
66
g.applyProps = (instance, oldProps, newProps) => {
77
const { draw, ...props } = newProps
8-
applyDefaultProps(instance, oldProps, props)
8+
let changed = applyDefaultProps(instance, oldProps, props)
99

1010
if (oldProps.draw !== draw && typeof draw === 'function') {
11+
changed = true
1112
draw.call(g, g)
1213
}
14+
15+
return changed
1316
}
1417

1518
return g

‎src/components/NineSlicePlane.js

+7-1
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,17 @@ const NineSlicePlane = (root, props) => {
99

1010
nineSlicePlane.applyProps = (instance, oldProps, newProps) => {
1111
const { image, texture, ...props } = newProps
12-
applyDefaultProps(instance, oldProps, props)
12+
let changed = applyDefaultProps(instance, oldProps, props)
1313

1414
if (image || texture) {
15+
// change = true not required for image, getTextureFromProps will call update
16+
if (texture !== oldProps.texture) {
17+
changed = true
18+
}
1519
instance.texture = getTextureFromProps('NineSlicePlane', newProps)
1620
}
21+
22+
return changed
1723
}
1824

1925
return nineSlicePlane

‎src/components/SimpleMesh.js

+7-1
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,17 @@ const SimpleMesh = (root, props) => {
99

1010
simpleMesh.applyProps = (instance, oldProps, newProps) => {
1111
const { image, texture, ...props } = newProps
12-
applyDefaultProps(instance, oldProps, props)
12+
let changed = applyDefaultProps(instance, oldProps, props)
1313

1414
if (image || texture) {
15+
// change = true not required for image, getTextureFromProps will call update
16+
if (texture !== oldProps.texture) {
17+
changed = true
18+
}
1519
instance.texture = getTextureFromProps('Mesh', newProps)
1620
}
21+
22+
return changed
1723
}
1824

1925
return simpleMesh

‎src/components/SimpleRope.js

+6-1
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,16 @@ const SimpleRope = (root, props) => {
1212
const { image, texture, ...props } = newProps
1313

1414
invariant(Array.isArray(newProps.points), 'SimpleRope points needs to be %s', 'Array<PIXI.Point>')
15-
applyDefaultProps(instance, oldProps, props)
15+
let changed = applyDefaultProps(instance, oldProps, props)
1616

1717
if (image || texture) {
18+
if (texture !== oldProps.texture) {
19+
changed = true
20+
}
1821
instance.texture = getTextureFromProps('SimpleRope', newProps)
1922
}
23+
24+
return changed
2025
}
2126

2227
return rope

‎src/components/Sprite.js

+7-1
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,17 @@ const Sprite = (root, props) => {
66

77
sprite.applyProps = (instance, oldProps, newProps) => {
88
const { image, texture, ...props } = newProps
9-
applyDefaultProps(instance, oldProps, props)
9+
let changed = applyDefaultProps(instance, oldProps, props)
1010

1111
if ((texture && oldProps.texture !== newProps.texture) || (image && oldProps.image !== newProps.image)) {
12+
// getTextureFromProps will call update for image
13+
if (oldProps.texture !== newProps.texture) {
14+
changed = true
15+
}
1216
instance.texture = getTextureFromProps('Sprite', newProps)
1317
}
18+
19+
return changed
1420
}
1521

1622
return sprite

‎src/components/TilingSprite.js

+15-4
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { TilingSprite as PixiTilingSprite } from 'pixi.js'
22
import { getTextureFromProps, applyDefaultProps } from '../utils/props'
3-
import { parsePoint } from '../utils/pixi'
3+
import { parsePoint, pointsAreEqual } from '../utils/pixi'
44

55
const TilingSprite = (root, props) => {
66
const { width = 100, height = 100 } = props
@@ -10,19 +10,30 @@ const TilingSprite = (root, props) => {
1010

1111
ts.applyProps = (instance, oldProps, newProps) => {
1212
const { tileScale, tilePosition, image, texture, ...props } = newProps
13-
applyDefaultProps(instance, oldProps, props)
13+
let changed = applyDefaultProps(instance, oldProps, props)
1414

1515
if (tilePosition) {
16-
instance.tilePosition.set(...parsePoint(tilePosition))
16+
const newTilePosition = parsePoint(tilePosition)
17+
instance.tilePosition.set(...newTilePosition)
18+
changed = !pointsAreEqual(parsePoint(oldProps.tilePosition), newTilePosition) || changed
1719
}
1820

1921
if (tileScale) {
20-
instance.tileScale.set(...parsePoint(tileScale))
22+
const newTileScale = parsePoint(tileScale)
23+
instance.tileScale.set(...newTileScale)
24+
changed = !pointsAreEqual(parsePoint(oldProps.tileScale), newTileScale) || changed
2125
}
2226

2327
if (image || texture) {
28+
// change = true not required for image, getTextureFromProps will call update
29+
if (texture !== oldProps.texture) {
30+
changed = true
31+
}
32+
2433
instance.texture = getTextureFromProps('Sprite', newProps)
2534
}
35+
36+
return changed
2637
}
2738

2839
return ts

‎src/utils/pixi.js

+21
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,27 @@ export function parsePoint(value) {
3030
return arr.filter(p => !isNil(p) && !isNaN(p)).map(Number)
3131
}
3232

33+
/**
34+
* Check if two points are equal
35+
*
36+
* @param {*} oldValue
37+
* @param {*} newValue
38+
* @returns {boolean}
39+
*/
40+
export function pointsAreEqual(oldValue, newValue) {
41+
if (oldValue.length !== newValue.length) {
42+
return false
43+
}
44+
45+
for (let i = 0; i < oldValue.length; i++) {
46+
if (oldValue[i] !== newValue[i]) {
47+
return false
48+
}
49+
}
50+
51+
return true
52+
}
53+
3354
/**
3455
* Determine value is type of Point or ObservablePoint
3556
* See https://github.com/michalochman/react-pixi-fiber/blob/a4dbddbef0ffbf0f563c64d30766ea28222a51ea/src/utils.js#L48

‎test/element.spec.js

+58-10
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,16 @@ describe('element.applyProps', () => {
116116
const element = createElement(TYPES.Sprite, { image: './image.png' })
117117
expect(spy).lastCalledWith('./image.png')
118118

119-
element.applyProps(element, { image: './image.png' }, { image: './new-image.png' })
119+
const changed = element.applyProps(element, { image: './image.png' }, { image: './new-image.png' })
120120
expect(spy).lastCalledWith('./new-image.png')
121+
expect(changed).toBeFalsy()
122+
})
123+
124+
test('Sprite.applyProps texture', () => {
125+
const element = createElement(TYPES.Sprite, { texture: emptyTexture })
126+
127+
const changed = element.applyProps(element, { texture: emptyTexture }, { image: './image.png' })
128+
expect(changed).toBeTruthy()
121129
})
122130

123131
test('TilingSprite.applyProps exists', () => {
@@ -130,10 +138,28 @@ describe('element.applyProps', () => {
130138
const element = createElement(TYPES.TilingSprite, { image: './image.png' })
131139
expect(spy).lastCalledWith('./image.png')
132140

133-
element.applyProps(element, { image: './image.png' }, { image: './new-image.png' })
141+
const changed = element.applyProps(element, { image: './image.png' }, { image: './new-image.png' })
142+
expect(changed).toBeFalsy()
134143
expect(spy).lastCalledWith('./new-image.png')
135144
})
136145

146+
test('TilingSprite.applyProps texture', () => {
147+
const element = createElement(TYPES.TilingSprite, { texture: emptyTexture })
148+
149+
const changed = element.applyProps(element, { texture: emptyTexture }, { image: './image.png' })
150+
expect(changed).toBeTruthy()
151+
})
152+
153+
test('TilingSprite.applyProps tilePosition', () => {
154+
const oldPosition = '1, 2'
155+
const newPosition = {x: 12, y: 20}
156+
const element = createElement(TYPES.TilingSprite, { tilePosition: oldPosition, image: './image.png' })
157+
158+
const changed = element.applyProps(element, { tilePosition: oldPosition, image: './image.png' }, { tilePosition: newPosition, image: './image.png' })
159+
expect(changed).toBeTruthy()
160+
})
161+
162+
137163
test('SimpleRope.applyProps exists', () => {
138164
const element = createElement(TYPES.SimpleRope, {
139165
image: './image.png',
@@ -150,7 +176,7 @@ describe('element.applyProps', () => {
150176
})
151177
expect(spy).lastCalledWith('./image.png')
152178

153-
element.applyProps(
179+
const changed = element.applyProps(
154180
element,
155181
{ image: './image.png' },
156182
{
@@ -159,6 +185,7 @@ describe('element.applyProps', () => {
159185
}
160186
)
161187
expect(spy).lastCalledWith('./new-image.png')
188+
expect(changed).toBeTruthy()
162189
})
163190

164191
test('NineSlicePlane.applyProps exists', () => {
@@ -171,8 +198,15 @@ describe('element.applyProps', () => {
171198
const element = createElement(TYPES.NineSlicePlane, { image: './image.png' })
172199
expect(spy).lastCalledWith('./image.png')
173200

174-
element.applyProps(element, { image: './image.png' }, { image: './new-image.png' })
201+
const changed = element.applyProps(element, { image: './image.png' }, { image: './new-image.png' })
175202
expect(spy).lastCalledWith('./new-image.png')
203+
expect(changed).toBeFalsy()
204+
})
205+
206+
test('NineSlicePlane.applyProps texture', () => {
207+
const element = createElement(TYPES.NineSlicePlane, { texture: emptyTexture })
208+
const changed = element.applyProps(element, { texture: emptyTexture }, { image: './new-image.png' })
209+
expect(changed).toBeTruthy()
176210
})
177211

178212
test('SimpleMesh.applyProps exists', () => {
@@ -185,8 +219,16 @@ describe('element.applyProps', () => {
185219
const element = createElement(TYPES.SimpleMesh, { image: './image.png' })
186220
expect(spy).lastCalledWith('./image.png')
187221

188-
element.applyProps(element, { image: './image.png' }, { image: './new-image.png' })
222+
let changed = element.applyProps(element, { image: './image.png' }, { image: './new-image.png' })
189223
expect(spy).lastCalledWith('./new-image.png')
224+
expect(changed).toBeFalsy()
225+
})
226+
227+
test('SimpleMesh.applyProps texture', () => {
228+
const element = createElement(TYPES.SimpleMesh, { texture: emptyTexture })
229+
230+
let changed = element.applyProps(element, { texture: emptyTexture }, { image: './new-image.png' })
231+
expect(changed).toBeTruthy()
190232
})
191233

192234
test('Graphics.applyProps exists', () => {
@@ -203,21 +245,27 @@ describe('element.applyProps', () => {
203245
const element = createElement(TYPES.Graphics, { draw: spy })
204246
expect(spy).toHaveBeenCalledTimes(1)
205247

206-
element.applyProps(element, { draw: spy }, { draw: spy })
248+
const applied = element.applyProps(element, { draw: spy }, { draw: spy })
207249
expect(spy).toHaveBeenCalledTimes(1)
250+
expect(applied).toBeFalsy()
208251
})
209252

210253
test('Graphics.applyProps draw prevented twice', () => {
211254
const draw1 = jest.fn()
212255
const draw2 = jest.fn()
213256
const props = { draw: draw1 }
214257
const nextProps = { draw: draw2 }
258+
let applied = false;
215259
const element = createElement(TYPES.Graphics, props)
216-
element.applyProps(element, props, props)
217-
element.applyProps(element, props, props)
260+
applied = element.applyProps(element, props, props)
261+
expect(applied).toBeFalsy()
262+
applied = element.applyProps(element, props, props)
263+
expect(applied).toBeFalsy()
218264
expect(draw1).toHaveBeenCalledTimes(1)
219-
element.applyProps(element, props, nextProps)
220-
element.applyProps(element, nextProps, nextProps)
265+
applied = element.applyProps(element, props, nextProps)
266+
expect(applied).toBeTruthy()
267+
applied = element.applyProps(element, nextProps, nextProps)
268+
expect(applied).toBeFalsy()
221269
expect(draw2).toHaveBeenCalledTimes(1)
222270
})
223271
})

0 commit comments

Comments
 (0)