Skip to content
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

Framework: add schema for sharing/publicize #3357

Merged
merged 1 commit into from
Mar 8, 2016

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Feb 17, 2016

This PR adds a schema for sharing/publicize so we can avoid data shape changes as described in #3101.

Thanks @aduth's refactors in #3417, we only need to persist connections. Other transient data like fetchingConnections will never be persisted.

Testing Instructions

  • In the console set localStorage.debug to calypso:state
  • Make sure that your blog has some connections ( Connected to Facebook/Twitter/etc ).
  • Navigate to the Editor
  • Refresh the page.
  • The app behaves normally and loads the persisted state/current-user from IndexedDB storage

To Validate that we can't get into an inconsistent state:

  • Add a non-existent required property to connectionsSchema, like foo:
export const connectionsSchema = {
    type: 'object',
    patternProperties: {
        '^\\d+$': {
            type: 'object',
            required: [ 'ID', 'site_ID', 'foo' ], //add a non-existent prop to test invalidations
            properties: {
                ID: { type: 'integer' },
                site_ID: { type: 'integer' },
                //...
            }
        }
    },
    additionalProperties: false
};
  • Navigate to the Editor
  • Refresh the page
  • You should see a console warning that validation failed
  • The Editor should load.

cc @aduth @mtias @rralian @blowery

@gwwar gwwar added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 17, 2016
@gwwar gwwar self-assigned this Feb 17, 2016
@gwwar gwwar added this to the Calypso Core: Offline 4 milestone Feb 17, 2016
@@ -57,7 +65,7 @@ export function connections( state = {}, action ) {
case SERIALIZE:
return state;
case DESERIALIZE:
return state;
return isValidStateWithSchema( state, connectionsSchema ) ? state : {};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming line length is what drove you to using ternary in this case and condition blocks in the others, though from a legibility perspective, I think we should be consistent (on the block?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 We can stick with blocks. Long ternaries tend to be more error prone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in ba8b593

@aduth aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 17, 2016
@gwwar gwwar force-pushed the add/persist-sharing-publicize branch from ca8cd3b to ba8b593 Compare February 18, 2016 02:12
@gwwar gwwar force-pushed the add/persist-sharing-publicize branch from ba8b593 to 446518b Compare February 18, 2016 22:19
@gwwar gwwar added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 18, 2016
@blowery
Copy link
Contributor

blowery commented Feb 19, 2016

Perhaps connectionsBySiteId should be a selector instead of stored data?

@gwwar
Copy link
Contributor Author

gwwar commented Feb 19, 2016

Perhaps connectionsBySiteId should be a selector instead of stored data?

Interesting, I'll play around with that. I would like to avoid a larger schema object if possible.

@gwwar gwwar added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 19, 2016
@aduth
Copy link
Contributor

aduth commented Feb 19, 2016

Perhaps connectionsBySiteId should be a selector instead of stored data?

Ah yeah, I knew there was something about this subtree I meant to bring in line with our recommendations. We should definitely make this change.

@gwwar : Do you think it should be made separately?

@gwwar
Copy link
Contributor Author

gwwar commented Feb 19, 2016

🙇 Thanks @aduth I'll update this PR after #3417 lands

@gwwar gwwar force-pushed the add/persist-sharing-publicize branch from 7e699af to 19f173d Compare February 22, 2016 19:49
@gwwar gwwar added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 22, 2016
@gwwar gwwar force-pushed the add/persist-sharing-publicize branch 2 times, most recently from 6b04f85 to dfcd237 Compare February 27, 2016 01:14
@gwwar
Copy link
Contributor Author

gwwar commented Feb 27, 2016

cc @aduth @rralian @blowery almost forgot about this one. It's ready for another look if you have time.

external_display: { type: 'string' },
external_profile_picture: { type: 'string' },
external_profile_URL: { type: 'string' },
external_follower_count: { type: 'integer' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting a schema validation warning here. This value is null in my case.

image

@aduth aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 29, 2016
@gwwar
Copy link
Contributor Author

gwwar commented Feb 29, 2016

Thanks @aduth I'll take another look

@gwwar gwwar force-pushed the add/persist-sharing-publicize branch from dfcd237 to cd6cbe8 Compare March 8, 2016 18:21
@gwwar gwwar added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 8, 2016
@gwwar
Copy link
Contributor Author

gwwar commented Mar 8, 2016

@aduth I've updated the schema to allow null types for all external properties.

@aduth
Copy link
Contributor

aduth commented Mar 8, 2016

Looks good 👍

(Side-note: I'm really excited for state persistence to gain more adoption / become enabled in production 😄 )

@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 8, 2016
@gwwar
Copy link
Contributor Author

gwwar commented Mar 8, 2016

Thanks for the review! 😄 Devs should be a bit more aware of it when #3853 enables it in stage. I'll continue to chip away at these schema PRs in my spare time.

@gwwar gwwar force-pushed the add/persist-sharing-publicize branch from cd6cbe8 to f725bd4 Compare March 8, 2016 21:07
gwwar added a commit that referenced this pull request Mar 8, 2016
Framework: add schema for sharing/publicize
@gwwar gwwar merged commit 0b2c11a into master Mar 8, 2016
@gwwar gwwar deleted the add/persist-sharing-publicize branch March 8, 2016 21:16
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.

5 participants