From f60c2871d30b381fb5f3315c4de77ccdd1650676 Mon Sep 17 00:00:00 2001 From: Vladislav Shkodin Date: Wed, 23 Dec 2020 13:53:50 +0200 Subject: [PATCH] refactor: remove immutable from medias state slice (#4740) --- packages/netlify-cms-core/package.json | 1 + .../{media.spec.js => media.spec.ts} | 73 ++++++++++++---- .../netlify-cms-core/src/actions/media.ts | 23 +++-- .../src/reducers/__tests__/medias.spec.ts | 33 ++++---- .../netlify-cms-core/src/reducers/medias.ts | 84 +++++++++++-------- packages/netlify-cms-core/src/types/redux.ts | 10 +-- yarn.lock | 7 ++ 7 files changed, 144 insertions(+), 87 deletions(-) rename packages/netlify-cms-core/src/actions/__tests__/{media.spec.js => media.spec.ts} (57%) diff --git a/packages/netlify-cms-core/package.json b/packages/netlify-cms-core/package.json index 5b549074..fb978ed5 100644 --- a/packages/netlify-cms-core/package.json +++ b/packages/netlify-cms-core/package.json @@ -86,6 +86,7 @@ "react-immutable-proptypes": "^2.1.0" }, "devDependencies": { + "@types/redux-mock-store": "^1.0.2", "redux-mock-store": "^1.5.3" } } diff --git a/packages/netlify-cms-core/src/actions/__tests__/media.spec.js b/packages/netlify-cms-core/src/actions/__tests__/media.spec.ts similarity index 57% rename from packages/netlify-cms-core/src/actions/__tests__/media.spec.js rename to packages/netlify-cms-core/src/actions/__tests__/media.spec.ts index f3575d60..9b77e13d 100644 --- a/packages/netlify-cms-core/src/actions/__tests__/media.spec.js +++ b/packages/netlify-cms-core/src/actions/__tests__/media.spec.ts @@ -1,11 +1,18 @@ import { Map } from 'immutable'; -import { getAsset, ADD_ASSET, LOAD_ASSET_REQUEST } from '../media'; import configureMockStore from 'redux-mock-store'; -import thunk from 'redux-thunk'; +import thunk, { ThunkDispatch } from 'redux-thunk'; +import { AnyAction } from 'redux'; +import { mocked } from 'ts-jest/utils'; +import { getAsset, ADD_ASSET, LOAD_ASSET_REQUEST } from '../media'; +import { selectMediaFilePath } from '../../reducers/entries'; +import { State } from '../../types/redux'; import AssetProxy from '../../valueObjects/AssetProxy'; const middlewares = [thunk]; -const mockStore = configureMockStore(middlewares); +const mockStore = configureMockStore, ThunkDispatch>( + middlewares, +); +const mockedSelectMediaFilePath = mocked(selectMediaFilePath); jest.mock('../../reducers/entries'); jest.mock('../mediaLibrary'); @@ -19,10 +26,10 @@ describe('media', () => { }); describe('getAsset', () => { + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore global.URL = { createObjectURL: jest.fn() }; - const { selectMediaFilePath } = require('../../reducers/entries'); - beforeEach(() => { jest.resetAllMocks(); }); @@ -30,8 +37,12 @@ describe('media', () => { it('should return empty asset for null path', () => { const store = mockStore({}); - const payload = { collection: null, entryPath: null, path: null }; + const payload = { collection: null, entryPath: null, entry: null, path: null }; + // TODO change to proper payload when immutable is removed + // from 'collections' and 'entries' state slices + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore const result = store.dispatch(getAsset(payload)); const actions = store.getActions(); expect(actions).toHaveLength(0); @@ -42,22 +53,30 @@ describe('media', () => { const path = 'static/media/image.png'; const asset = new AssetProxy({ file: new File([], 'empty'), path }); const store = mockStore({ + // TODO change to proper store data when immutable is removed + // from 'config' state slice + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore config: Map(), - medias: Map({ - [path]: { asset }, - }), + medias: { + [path]: { asset, isLoading: false, error: null }, + }, }); - selectMediaFilePath.mockReturnValue(path); + mockedSelectMediaFilePath.mockReturnValue(path); const payload = { collection: Map(), entry: Map({ path: 'entryPath' }), path }; + // TODO change to proper payload when immutable is removed + // from 'collections' and 'entries' state slices + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore const result = store.dispatch(getAsset(payload)); const actions = store.getActions(); expect(actions).toHaveLength(0); expect(result).toBe(asset); - expect(selectMediaFilePath).toHaveBeenCalledTimes(1); - expect(selectMediaFilePath).toHaveBeenCalledWith( + expect(mockedSelectMediaFilePath).toHaveBeenCalledTimes(1); + expect(mockedSelectMediaFilePath).toHaveBeenCalledWith( store.getState().config, payload.collection, payload.entry, @@ -71,12 +90,16 @@ describe('media', () => { const asset = new AssetProxy({ url: path, path }); const store = mockStore({ - medias: Map({}), + medias: {}, }); - selectMediaFilePath.mockReturnValue(path); + mockedSelectMediaFilePath.mockReturnValue(path); const payload = { collection: null, entryPath: null, path }; + // TODO change to proper payload when immutable is removed + // from 'collections' state slice + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore const result = store.dispatch(getAsset(payload)); const actions = store.getActions(); expect(actions).toHaveLength(1); @@ -90,12 +113,16 @@ describe('media', () => { it('should return empty asset and initiate load when not in medias state', () => { const path = 'static/media/image.png'; const store = mockStore({ - medias: Map({}), + medias: {}, }); - selectMediaFilePath.mockReturnValue(path); + mockedSelectMediaFilePath.mockReturnValue(path); const payload = { path }; + // TODO change to proper payload when immutable is removed + // from 'collections' and 'entries' state slices + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore const result = store.dispatch(getAsset(payload)); const actions = store.getActions(); expect(actions).toHaveLength(1); @@ -110,12 +137,22 @@ describe('media', () => { const path = 'static/media/image.png'; const resolvePath = 'resolvePath'; const store = mockStore({ - medias: Map({ [resolvePath]: { error: true } }), + medias: { + [resolvePath]: { + asset: undefined, + error: new Error('test'), + isLoading: false, + }, + }, }); - selectMediaFilePath.mockReturnValue(resolvePath); + mockedSelectMediaFilePath.mockReturnValue(resolvePath); const payload = { path }; + // TODO change to proper payload when immutable is removed + // from 'collections' and 'entries' state slices + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore const result = store.dispatch(getAsset(payload)); const actions = store.getActions(); diff --git a/packages/netlify-cms-core/src/actions/media.ts b/packages/netlify-cms-core/src/actions/media.ts index 4c985a76..3d4033b9 100644 --- a/packages/netlify-cms-core/src/actions/media.ts +++ b/packages/netlify-cms-core/src/actions/media.ts @@ -16,27 +16,27 @@ export const LOAD_ASSET_SUCCESS = 'LOAD_ASSET_SUCCESS'; export const LOAD_ASSET_FAILURE = 'LOAD_ASSET_FAILURE'; export function addAssets(assets: AssetProxy[]) { - return { type: ADD_ASSETS, payload: assets }; + return { type: ADD_ASSETS, payload: assets } as const; } export function addAsset(assetProxy: AssetProxy) { - return { type: ADD_ASSET, payload: assetProxy }; + return { type: ADD_ASSET, payload: assetProxy } as const; } export function removeAsset(path: string) { - return { type: REMOVE_ASSET, payload: path }; + return { type: REMOVE_ASSET, payload: path } as const; } export function loadAssetRequest(path: string) { - return { type: LOAD_ASSET_REQUEST, payload: { path } }; + return { type: LOAD_ASSET_REQUEST, payload: { path } } as const; } export function loadAssetSuccess(path: string) { - return { type: LOAD_ASSET_SUCCESS, payload: { path } }; + return { type: LOAD_ASSET_SUCCESS, payload: { path } } as const; } export function loadAssetFailure(path: string, error: Error) { - return { type: LOAD_ASSET_FAILURE, payload: { path, error } }; + return { type: LOAD_ASSET_FAILURE, payload: { path, error } } as const; } export function loadAsset(resolvedPath: string) { @@ -97,7 +97,7 @@ export function getAsset({ collection, entry, path, field }: GetAssetArgs) { const state = getState(); const resolvedPath = selectMediaFilePath(state.config, collection, entry, path, field); - let { asset, isLoading, error } = state.medias.get(resolvedPath) || {}; + let { asset, isLoading, error } = state.medias[resolvedPath] || {}; if (isLoading) { return emptyAsset; } @@ -125,3 +125,12 @@ export function getAsset({ collection, entry, path, field }: GetAssetArgs) { return asset; }; } + +export type MediasAction = ReturnType< + | typeof addAssets + | typeof addAsset + | typeof removeAsset + | typeof loadAssetRequest + | typeof loadAssetSuccess + | typeof loadAssetFailure +>; diff --git a/packages/netlify-cms-core/src/reducers/__tests__/medias.spec.ts b/packages/netlify-cms-core/src/reducers/__tests__/medias.spec.ts index 191cef0b..8eb40d6b 100644 --- a/packages/netlify-cms-core/src/reducers/__tests__/medias.spec.ts +++ b/packages/netlify-cms-core/src/reducers/__tests__/medias.spec.ts @@ -1,4 +1,3 @@ -import { Map, fromJS } from 'immutable'; import { addAssets, addAsset, @@ -14,37 +13,37 @@ describe('medias', () => { const asset = createAssetProxy({ url: 'url', path: 'path' }); it('should add assets', () => { - expect(reducer(fromJS({}), addAssets([asset]))).toEqual( - Map({ path: { asset, isLoading: false, error: null } }), - ); + expect(reducer({}, addAssets([asset]))).toEqual({ + path: { asset, isLoading: false, error: null }, + }); }); it('should add asset', () => { - expect(reducer(fromJS({}), addAsset(asset))).toEqual( - Map({ path: { asset, isLoading: false, error: null } }), - ); + expect(reducer({}, addAsset(asset))).toEqual({ + path: { asset, isLoading: false, error: null }, + }); }); it('should remove asset', () => { - expect(reducer(fromJS({ path: asset }), removeAsset(asset.path))).toEqual(Map()); + expect( + reducer({ [asset.path]: { asset, isLoading: false, error: null } }, removeAsset(asset.path)), + ).toEqual({}); }); it('should mark asset as loading', () => { - expect(reducer(fromJS({}), loadAssetRequest(asset.path))).toEqual( - Map({ path: { isLoading: true } }), - ); + expect(reducer({}, loadAssetRequest(asset.path))).toEqual({ path: { isLoading: true } }); }); it('should mark asset as not loading', () => { - expect(reducer(fromJS({}), loadAssetSuccess(asset.path))).toEqual( - Map({ path: { isLoading: false, error: null } }), - ); + expect(reducer({}, loadAssetSuccess(asset.path))).toEqual({ + path: { isLoading: false, error: null }, + }); }); it('should set loading error', () => { const error = new Error('some error'); - expect(reducer(fromJS({}), loadAssetFailure(asset.path, error))).toEqual( - Map({ path: { isLoading: false, error } }), - ); + expect(reducer({}, loadAssetFailure(asset.path, error))).toEqual({ + path: { isLoading: false, error }, + }); }); }); diff --git a/packages/netlify-cms-core/src/reducers/medias.ts b/packages/netlify-cms-core/src/reducers/medias.ts index 5c4674ba..0b6f2649 100644 --- a/packages/netlify-cms-core/src/reducers/medias.ts +++ b/packages/netlify-cms-core/src/reducers/medias.ts @@ -1,4 +1,4 @@ -import { fromJS } from 'immutable'; +import { produce } from 'immer'; import { ADD_ASSETS, ADD_ASSET, @@ -6,46 +6,58 @@ import { LOAD_ASSET_REQUEST, LOAD_ASSET_SUCCESS, LOAD_ASSET_FAILURE, + MediasAction, } from '../actions/media'; import AssetProxy from '../valueObjects/AssetProxy'; -import { Medias, MediasAction } from '../types/redux'; -const medias = (state: Medias = fromJS({}), action: MediasAction) => { - switch (action.type) { - case ADD_ASSETS: { - const payload = action.payload as AssetProxy[]; - let newState = state; - payload.forEach(asset => { - newState = newState.set(asset.path, { asset, isLoading: false, error: null }); - }); - return newState; - } - case ADD_ASSET: { - const asset = action.payload as AssetProxy; - return state.set(asset.path, { asset, isLoading: false, error: null }); - } - case REMOVE_ASSET: { - const payload = action.payload as string; - return state.delete(payload); - } - case LOAD_ASSET_REQUEST: { - const { path } = action.payload as { path: string }; - return state.set(path, { ...state.get(path), isLoading: true }); - } - case LOAD_ASSET_SUCCESS: { - const { path } = action.payload as { path: string }; - return state.set(path, { ...state.get(path), isLoading: false, error: null }); - } - case LOAD_ASSET_FAILURE: { - const { path, error } = action.payload as { path: string; error: Error }; - return state.set(path, { ...state.get(path), isLoading: false, error }); - } - default: - return state; - } +export type Medias = { + [path: string]: { asset: AssetProxy | undefined; isLoading: boolean; error: Error | null }; }; +const defaultState: Medias = {}; + +const medias = produce((state: Medias, action: MediasAction) => { + switch (action.type) { + case ADD_ASSETS: { + const assets = action.payload; + assets.forEach(asset => { + state[asset.path] = { asset, isLoading: false, error: null }; + }); + break; + } + case ADD_ASSET: { + const asset = action.payload; + state[asset.path] = { asset, isLoading: false, error: null }; + break; + } + case REMOVE_ASSET: { + const path = action.payload; + delete state[path]; + break; + } + case LOAD_ASSET_REQUEST: { + const { path } = action.payload; + state[path] = state[path] || {}; + state[path].isLoading = true; + break; + } + case LOAD_ASSET_SUCCESS: { + const { path } = action.payload; + state[path] = state[path] || {}; + state[path].isLoading = false; + state[path].error = null; + break; + } + case LOAD_ASSET_FAILURE: { + const { path, error } = action.payload; + state[path] = state[path] || {}; + state[path].isLoading = false; + state[path].error = error; + } + } +}, defaultState); + export const selectIsLoadingAsset = (state: Medias) => - Object.values(state.toJS()).some(state => state.isLoading); + Object.values(state).some(state => state.isLoading); export default medias; diff --git a/packages/netlify-cms-core/src/types/redux.ts b/packages/netlify-cms-core/src/types/redux.ts index 5bc8f3c6..d723b78c 100644 --- a/packages/netlify-cms-core/src/types/redux.ts +++ b/packages/netlify-cms-core/src/types/redux.ts @@ -1,10 +1,10 @@ import { Action } from 'redux'; import { StaticallyTypedRecord } from './immutable'; import { Map, List, OrderedMap, Set } from 'immutable'; -import AssetProxy from '../valueObjects/AssetProxy'; import { MediaFile as BackendMediaFile } from '../backend'; import { Auth } from '../reducers/auth'; import { Status } from '../reducers/status'; +import { Medias } from '../reducers/medias'; export type SlugConfig = StaticallyTypedRecord<{ encoding: string; @@ -226,10 +226,6 @@ export type Collection = StaticallyTypedRecord; export type Collections = StaticallyTypedRecord<{ [path: string]: Collection & CollectionObject }>; -export type Medias = StaticallyTypedRecord<{ - [path: string]: { asset: AssetProxy | undefined; isLoading: boolean; error: Error | null }; -}>; - export interface MediaLibraryInstance { show: (args: { id?: string; @@ -308,10 +304,6 @@ export interface State { status: Status; } -export interface MediasAction extends Action { - payload: string | AssetProxy | AssetProxy[] | { path: string } | { path: string; error: Error }; -} - export interface ConfigAction extends Action { payload: Map; } diff --git a/yarn.lock b/yarn.lock index 3765e4f0..5873d68b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3484,6 +3484,13 @@ "@types/prop-types" "*" csstype "^3.0.2" +"@types/redux-mock-store@^1.0.2": + version "1.0.2" + resolved "https://registry.yarnpkg.com/@types/redux-mock-store/-/redux-mock-store-1.0.2.tgz#c27d5deadfb29d8514bdb0fc2cadae6feea1922d" + integrity sha512-6LBtAQBN34i7SI5X+Qs4zpTEZO1tTDZ6sZ9fzFjYwTl3nLQXaBtwYdoV44CzNnyKu438xJ1lSIYyw0YMvunESw== + dependencies: + redux "^4.0.5" + "@types/serve-static@*": version "1.13.8" resolved "https://registry.yarnpkg.com/@types/serve-static/-/serve-static-1.13.8.tgz#851129d434433c7082148574ffec263d58309c46"