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

Posts: Add single site post requesting behavior to Redux state #3108

Merged
merged 3 commits into from
Feb 5, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Feb 5, 2016

This pull request seeks to implement Redux state behaviors for managing network requests of single site post objects. This will be used as part of the editor Redux migration effort in allowing the editor to reconcile post revisions with the persisted post entity.

Testing instructions:

There are no effective visual changes to test.

Ensure Mocha tests pass by running make test in your Terminal from the project root directory or directly from client/state/posts.

@aduth aduth added Posts [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 5, 2016
@aduth aduth added this to the Calypso Core: Offline 3 milestone Feb 5, 2016
@blowery
Copy link
Contributor

blowery commented Feb 5, 2016

Test pass locally, woo

@@ -23,6 +23,9 @@ export const FETCH_SITE_PLANS = 'FETCH_SITE_PLANS';
export const FETCH_SITE_PLANS_COMPLETED = 'FETCH_SITE_PLANS_COMPLETED';
export const FETCH_WPORG_PLUGIN_DATA = 'FETCH_WPORG_PLUGIN_DATA';
export const NEW_NOTICE = 'NEW_NOTICE';
export const POST_REQUEST = 'POST_REQUEST';
export const POST_REQUEST_SUCCESS = 'POST_REQUEST_SUCCESS';
export const POST_REQUEST_FAILURE = 'POST_REQUEST_FAILURE';
Copy link
Contributor

Choose a reason for hiding this comment

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

(note: not really for this PR)

I wonder if we should flip all of these to keymirror... It turns out the const doesn't transfer through the export, so an importer can still reassign the imported variable. I feel like the const is giving a false sense of security.

But maybe it helps with code completion in some editors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we should flip all of these to keymirror

What advantage would there be in using keyMirror?

For me at least, this is less about feeling of security with const and rather in opting to use simple language constructs than an external library that, in my opinion, doesn't provide much value.

There's some discussion in the keyMirror repository about minifiers crushing keys, though I don't think it affects our situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

just less duplication. it's really easy to fat finger one side of the const construction and not notice.

MY_REALLY_LONG_KEY = "MY_REALY_LONG_KEY"

vs

keyMirror( { MY_REALLY_LONG_KEY: t } )

I'm not worried about the minifier advantages really. Though that could speed up string compares I guess. Likely meh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's really easy to fat finger one side of the const construction and not notice

Maybe some custom lint rules could help here?

Related: #3015, https://github.com/Automattic/eslint-plugin-wpcalypso

Bugs in reducer action handlers causing state to be mutated
unintentionally.
@aduth
Copy link
Contributor Author

aduth commented Feb 5, 2016

Thanks for the tip on deep-freeze, @blowery . Introduced it in 52ad4f8, which unveiled a few bugs in the existing posts reducers, fixed in the same commit.

Will definitely plan on using deep-freeze consistently in future tests.

@blowery
Copy link
Contributor

blowery commented Feb 5, 2016

Looks good, unleash the 🐲.

@blowery blowery 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 Feb 5, 2016
@aduth
Copy link
Contributor Author

aduth commented Feb 5, 2016

Thanks for the review, @blowery ! Some great discussion/discoveries in this pull request 😄

aduth added a commit that referenced this pull request Feb 5, 2016
Posts: Add single site post requesting behavior to Redux state
@aduth aduth merged commit 2a0c2b9 into master Feb 5, 2016
@aduth aduth deleted the add/redux-request-site-post branch February 5, 2016 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. Posts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants