Skip to content

Fix expensive operation in mapStateToProps#3250

Merged
fsih merged 2 commits intoscratchfoundation:developfrom
fsih:fixGetCostumeInMapStateToProps
Oct 3, 2018
Merged

Fix expensive operation in mapStateToProps#3250
fsih merged 2 commits intoscratchfoundation:developfrom
fsih:fixGetCostumeInMapStateToProps

Conversation

@fsih
Copy link
Contributor

@fsih fsih commented Sep 28, 2018

Resolves

Fixes #3235

Proposed Changes

Move getCostume() out of mapStateToProps. mapStateToProps is called any time the state changes, and getCostume can be very expensive because it has to decode from storage. In addition, I added a shouldComponentUpdate to ignore the props that don't affect rendering.

Reason for Changes

Projects could run very slowly when the paint editor was open

Test Coverage

Tested Space Giraffe and Geometry Dash

Browser Coverage

Check the OS/browser combinations tested (At least 2)

Mac

  • Chrome
  • Firefox
  • Safari

Windows

  • Chrome
  • Firefox
  • Edge

Chromebook

  • Chrome

iPad

  • Safari

Android Tablet

  • Chrome

@fsih fsih requested a review from chrisgarrity September 28, 2018 14:56
@fsih fsih added this to the October 2018 milestone Sep 28, 2018
Copy link
Contributor

@chrisgarrity chrisgarrity left a comment

Choose a reason for hiding this comment

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

Just one picky detail. I did run it and everything worked for me.

@fsih fsih force-pushed the fixGetCostumeInMapStateToProps branch from e64d090 to ea4e339 Compare October 3, 2018 17:21
Copy link
Contributor

@chrisgarrity chrisgarrity left a comment

Choose a reason for hiding this comment

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

LGTM!

@chrisgarrity chrisgarrity assigned fsih and unassigned chrisgarrity Oct 3, 2018
@fsih fsih merged commit a39e47f into scratchfoundation:develop Oct 3, 2018
@fsih fsih deleted the fixGetCostumeInMapStateToProps branch October 3, 2018 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants