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

Allow stored values to be updated with new defaults #2809

Merged
merged 2 commits into from Sep 28, 2017

Conversation

Projects
None yet
2 participants
@notnownikki
Member

notnownikki commented Sep 27, 2017

Description

If we store preferences, then we want to add new preferences
with new defaults, things break because the new defaults
were not applied, so the new preferences were not in place,
and we had to have code checking for undefined everywhere
we wanted to use the newly added preference.

This change applies defaults to stored data where the properties
do not already exist, allowing us to apply defaults to
stored data that was produced before we added the new preferences.

How Has This Been Tested?

Unit tests have been added.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.
Allow stored values to be updated with new defaults
If we store preferences, then we want to add new preferences
with new defaults, things broke because the new defaults
were not applied, so the new preferences were not in place,
and we had to have code checking for `undefined` everywhere
we wanted to use the newly added preference.

This change applies defaults to stored data where the properties
do not already exist, allowing us to apply defaults to
stored data that was produced before we added the new preferences.

@notnownikki notnownikki requested a review from youknowriad Sep 27, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 27, 2017

Codecov Report

Merging #2809 into master will increase coverage by 0.73%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2809      +/-   ##
==========================================
+ Coverage   33.73%   34.47%   +0.73%     
==========================================
  Files         190      191       +1     
  Lines        5676     5744      +68     
  Branches      991     1015      +24     
==========================================
+ Hits         1915     1980      +65     
- Misses       3183     3186       +3     
  Partials      578      578
Impacted Files Coverage Δ
editor/reducer.js 88.57% <ø> (-0.09%) ⬇️
editor/store-defaults.js 100% <100%> (ø)
editor/store-persist.js 100% <100%> (ø) ⬆️
editor/selectors.js 95% <0%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 345ca17...cfd39d7. Read the comment docs.

codecov bot commented Sep 27, 2017

Codecov Report

Merging #2809 into master will increase coverage by 0.73%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2809      +/-   ##
==========================================
+ Coverage   33.73%   34.47%   +0.73%     
==========================================
  Files         190      191       +1     
  Lines        5676     5744      +68     
  Branches      991     1015      +24     
==========================================
+ Hits         1915     1980      +65     
- Misses       3183     3186       +3     
  Partials      578      578
Impacted Files Coverage Δ
editor/reducer.js 88.57% <ø> (-0.09%) ⬇️
editor/store-defaults.js 100% <100%> (ø)
editor/store-persist.js 100% <100%> (ø) ⬆️
editor/selectors.js 95% <0%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 345ca17...cfd39d7. Read the comment docs.

@youknowriad

Left some minor notes, but agreed, this is something that should be fixed.

Nice work :)

@@ -0,0 +1,7 @@
export const STORE_DEFAULTS = {

This comment has been minimized.

@youknowriad

youknowriad Sep 27, 2017

Contributor

not sure if this should be in a separate file, what about just a module constant in store.js

@youknowriad

youknowriad Sep 27, 2017

Contributor

not sure if this should be in a separate file, what about just a module constant in store.js

This comment has been minimized.

@notnownikki

notnownikki Sep 27, 2017

Member

That's tricky, because store.js imports from store-persist.js, and if we moved it into store.js we end up with circular imports. So because we have the reducer importing it as well, a separate file seemed to be the cleanest way.

@notnownikki

notnownikki Sep 27, 2017

Member

That's tricky, because store.js imports from store-persist.js, and if we moved it into store.js we end up with circular imports. So because we have the reducer importing it as well, a separate file seemed to be the cleanest way.

This comment has been minimized.

@youknowriad

youknowriad Sep 27, 2017

Contributor

I guess this would create a cyclic dependency between store and reducer so maybe in reducer instead

@youknowriad

youknowriad Sep 27, 2017

Contributor

I guess this would create a cyclic dependency between store and reducer so maybe in reducer instead

This comment has been minimized.

@notnownikki

notnownikki Sep 27, 2017

Member

Seems weird to have store defaults in reducer to me

@notnownikki

notnownikki Sep 27, 2017

Member

Seems weird to have store defaults in reducer to me

This comment has been minimized.

@youknowriad

youknowriad Sep 27, 2017

Contributor

I'm fine with keeping it this way

@youknowriad

youknowriad Sep 27, 2017

Contributor

I'm fine with keeping it this way

Show outdated Hide outdated editor/store-persist.js Outdated
@youknowriad

Let's wait for the release to merge this.

Good job 👍

@notnownikki notnownikki merged commit fbf6382 into master Sep 28, 2017

3 checks passed

codecov/project 34.47% (+0.73%) compared to 345ca17
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/support-new-default-preferences branch Sep 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment