diff --git a/service/actions/posts.js b/service/actions/posts.js index ebfe02b76..9cd6d76cb 100644 --- a/service/actions/posts.js +++ b/service/actions/posts.js @@ -43,7 +43,7 @@ export function deletePost(teamId, post) { dispatch(batchActions([ { type: PostsTypes.POST_DELETED, - data: post + data: {...post} }, { type: PostsTypes.DELETE_POST_SUCCESS @@ -56,7 +56,7 @@ export function removePost(post) { return async (dispatch, getState) => { dispatch({ type: PostsTypes.REMOVE_POST, - data: post + data: {...post} }, getState); }; } @@ -77,7 +77,7 @@ export function getPost(teamId, channelId, postId) { dispatch(batchActions([ { type: PostsTypes.RECEIVED_POSTS, - data: post, + data: {...post}, channelId }, { diff --git a/service/reducers/entities/posts.js b/service/reducers/entities/posts.js index a95075f3a..712f93d8d 100644 --- a/service/reducers/entities/posts.js +++ b/service/reducers/entities/posts.js @@ -111,11 +111,14 @@ function deletePost(state, action) { if (postInfo.posts[post.id]) { const combinedPosts = makePostListNonNull(postInfo); - combinedPosts.posts[post.id] = { - ...post, - state: Constants.POST_DELETED, - file_ids: [], - has_reactions: false + combinedPosts.posts = { + ...combinedPosts.posts, + [post.id]: { + ...post, + state: Constants.POST_DELETED, + file_ids: [], + has_reactions: false + } }; return { @@ -143,6 +146,11 @@ function removePost(state, action) { if (postInfo.posts[post.id]) { const combinedPosts = makePostListNonNull(postInfo); + + combinedPosts.order = combinedPosts.order.slice(0); + combinedPosts.posts = {...combinedPosts.posts}; + + // Remove the post Reflect.deleteProperty(combinedPosts.posts, post.id); const index = combinedPosts.order.indexOf(post.id); @@ -150,11 +158,8 @@ function removePost(state, action) { combinedPosts.order.splice(index, 1); } - for (const pid in combinedPosts.posts) { - if (!combinedPosts.posts.hasOwnProperty(pid)) { - continue; - } - + // Remove any children of the post + for (const pid of Object.keys(combinedPosts.posts)) { if (combinedPosts.posts[pid].root_id === post.id) { Reflect.deleteProperty(combinedPosts.posts, pid); const commentIndex = combinedPosts.order.indexOf(pid); diff --git a/service/store/configureStore.dev.js b/service/store/configureStore.dev.js index c6b48031c..14bbe18f3 100644 --- a/service/store/configureStore.dev.js +++ b/service/store/configureStore.dev.js @@ -2,14 +2,16 @@ // See License.txt for license information. import {applyMiddleware, compose, createStore, combineReducers} from 'redux'; -import devTools from 'remote-redux-devtools'; import {enableBatching} from 'redux-batched-actions'; -import serviceReducer from 'service/reducers'; +import devTools from 'remote-redux-devtools'; import thunk from 'redux-thunk'; +import serviceReducer from 'service/reducers'; +import deepFreezeAndThrowOnMutation from 'service/utils/deepFreezeAndThrowOnMutation'; + export default function configureServiceStore(preloadedState, appReducer, getAppReducer) { const store = createStore( - enableBatching(combineReducers(Object.assign({}, serviceReducer, appReducer))), + createReducer(serviceReducer, appReducer), preloadedState, compose( applyMiddleware(thunk), @@ -29,9 +31,27 @@ export default function configureServiceStore(preloadedState, appReducer, getApp if (getAppReducer) { nextAppReducer = getAppReducer(); // eslint-disable-line global-require } - store.replaceReducer(combineReducers(Object.assign({}, nextServiceReducer, nextAppReducer))); + store.replaceReducer(createReducer(nextServiceReducer, nextAppReducer)); }); } return store; } + +function createReducer(...reducers) { + const baseReducer = combineReducers(Object.assign({}, ...reducers)); + + return enableFreezing(enableBatching(baseReducer)); +} + +function enableFreezing(reducer) { + return (state, action) => { + const nextState = reducer(state, action); + + if (nextState !== state) { + deepFreezeAndThrowOnMutation(nextState); + } + + return nextState; + }; +} diff --git a/service/utils/deepFreezeAndThrowOnMutation.js b/service/utils/deepFreezeAndThrowOnMutation.js new file mode 100644 index 000000000..4e9cdc7c0 --- /dev/null +++ b/service/utils/deepFreezeAndThrowOnMutation.js @@ -0,0 +1,60 @@ +/** + * Copyright (c) 2015-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +/** + * If your application is accepting different values for the same field over + * time and is doing a diff on them, you can either (1) create a copy or + * (2) ensure that those values are not mutated behind two passes. + * This function helps you with (2) by freezing the object and throwing if + * the user subsequently modifies the value. + * + * There are two caveats with this function: + * - If the call site is not in strict mode, it will only throw when + * mutating existing fields, adding a new one + * will unfortunately fail silently :( + * - If the object is already frozen or sealed, it will not continue the + * deep traversal and will leave leaf nodes unfrozen. + * + * Freezing the object and adding the throw mechanism is expensive and will + * only be used in DEV. + */ +export default function deepFreezeAndThrowOnMutation(object) { + if (typeof object !== 'object' || object === null || Object.isFrozen(object) || Object.isSealed(object)) { + return; + } + + for (const key in object) { + if (object.hasOwnProperty(key)) { + object.__defineGetter__(key, identity.bind(null, object[key])); // eslint-disable-line no-underscore-dangle + object.__defineSetter__(key, throwOnImmutableMutation.bind(null, key)); // eslint-disable-line no-underscore-dangle + } + } + + Object.freeze(object); + Object.seal(object); + + for (const key in object) { + if (object.hasOwnProperty(key)) { + deepFreezeAndThrowOnMutation(object[key]); + } + } +} + +function throwOnImmutableMutation(key, value) { + throw Error( + 'You attempted to set the key `' + key + '` with the value `' + + JSON.stringify(value) + '` on an object that is meant to be immutable ' + + 'and has been frozen.' + ); +} + +function identity(value) { + return value; +} diff --git a/test/service/actions/posts.test.js b/test/service/actions/posts.test.js index 535cb6d58..6aaed0be9 100644 --- a/test/service/actions/posts.test.js +++ b/test/service/actions/posts.test.js @@ -75,45 +75,36 @@ describe('Actions.Posts', () => { }); it('deletePost', (done) => { - TestHelper.initBasic(Client).then(async () => { + TestHelper.initBasic(Client).then(async ({channel}) => { const store = configureStore(); - let created; + + await Actions.createPost( + TestHelper.basicTeam.id, + TestHelper.fakePost(TestHelper.basicChannel.id) + )(store.dispatch, store.getState); + + const initialPostsInfo = store.getState().entities.posts.postsInfo[channel.id]; + const created = initialPostsInfo.posts[initialPostsInfo.order[0]]; store.subscribe(() => { - const createRequest = store.getState().requests.posts.createPost; - const deleteRequest = store.getState().requests.posts.deletePost; - const postsInfo = store.getState().entities.posts.postsInfo; + const state = store.getState(); + const deleteRequest = state.requests.posts.deletePost; + const postsInfo = state.entities.posts.postsInfo[channel.id]; - if (deleteRequest.status === RequestStatus.SUCCESS || deleteRequest.status === RequestStatus.FAILURE) { - if (deleteRequest.error) { - done(new Error(JSON.stringify(deleteRequest.error))); - } else { - assert.ok(postsInfo[TestHelper.basicChannel.id]); - assert.strictEqual( - postsInfo[TestHelper.basicChannel.id].posts[created.id].state, - Constants.POST_DELETED - ); - done(); - } - } + if (deleteRequest.status === RequestStatus.SUCCESS) { + assert.ok(postsInfo); + assert.strictEqual( + postsInfo.posts[created.id].state, + Constants.POST_DELETED + ); - if (deleteRequest.status === RequestStatus.NOT_STARTED && - (createRequest.status === RequestStatus.SUCCESS || createRequest.status === RequestStatus.FAILURE)) { - if (createRequest.error) { - done(new Error(JSON.stringify(createRequest.error))); - } else { - const posts = postsInfo[TestHelper.basicChannel.id].posts; - created = posts[Object.keys(posts)[0]]; - Actions.deletePost(TestHelper.basicTeam.id, created)(store.dispatch, store.getState); - } + done(); + } else if (deleteRequest.status === RequestStatus.FAILURE) { + done(new Error(JSON.stringify(deleteRequest.error))); } }); - const post = TestHelper.fakePost(TestHelper.basicChannel.id); - Actions.createPost( - TestHelper.basicTeam.id, - post - )(store.dispatch, store.getState); + Actions.deletePost(TestHelper.basicTeam.id, created)(store.dispatch, store.getState); }); }); @@ -133,9 +124,11 @@ describe('Actions.Posts', () => { store.subscribe(() => { const postsInfo = store.getState().entities.posts.postsInfo; const postList = postsInfo[TestHelper.basicChannel.id]; + assert.strictEqual(postList.order.length, 0); - assert.ifError(postList.posts[post1a.id]); - assert.ifError(postList.posts[TestHelper.basicPost.id]); + assert.ok(!postList.posts[post1a.id]); + assert.ok(!postList.posts[TestHelper.basicPost.id]); + done(); });