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.

Sign up for GitHub

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

refactor: remove immutable from 'config' state slice #4960

Merged
merged 8 commits into from
Mar 11, 2021
Merged

refactor: remove immutable from 'config' state slice #4960

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7892db8
refactor: remove immutable from 'config' state slice
smashercosmo Feb 15, 2021
637819d
fix: add type to config's state
smashercosmo Feb 17, 2021
57b96f6
fix: remove non public types
erezrokah Feb 28, 2021
84366c6
refactor: don't use default state in tests
erezrokah Feb 28, 2021
c4fa7e8
refactor: remove todos
erezrokah Feb 28, 2021
652bebf
refactor: code cleanup
erezrokah Mar 10, 2021
3e3afde
chore: fix lint error
erezrokah Mar 10, 2021
c0e43bc
fix: maintain collection config order
erezrokah Mar 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions packages/netlify-cms-core/src/reducers/__tests__/integrations.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import integrations from '../integrations';
import { defaultState as defaultConfig } from '../../reducers/config';
import { CONFIG_SUCCESS } from '../../actions/config';
import { CONFIG_SUCCESS, ConfigAction } from '../../actions/config';
import { FOLDER } from '../../constants/collectionTypes';

describe('integrations', () => {
it('should return default state when no integrations', () => {
const result = integrations(null, {
type: CONFIG_SUCCESS,
payload: { ...defaultConfig, integrations: [] },
});
payload: { integrations: [] },
} as ConfigAction);
expect(result && result.toJS()).toEqual({
providers: {},
hooks: {},
Expand All @@ -19,7 +18,6 @@ describe('integrations', () => {
const result = integrations(null, {
type: CONFIG_SUCCESS,
payload: {
...defaultConfig,
integrations: [
{
hooks: ['listEntries'],
Expand Down Expand Up @@ -47,7 +45,7 @@ describe('integrations', () => {
{ name: 'faq', label: 'FAQ', type: FOLDER },
],
},
});
} as ConfigAction);

expect(result && result.toJS()).toEqual({
providers: {
Expand Down
7 changes: 3 additions & 4 deletions packages/netlify-cms-core/src/reducers/collections.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,9 @@ function collections(state = defaultState, action: ConfigAction) {
switch (action.type) {
case CONFIG_SUCCESS: {
const collections = action.payload.collections;
const newState: { [name: string]: CmsCollection } = {};
for (const collection of collections) {
newState[collection.name] = collection;
}
const newState = Object.fromEntries(
Copy link
Contributor Author

@smashercosmo smashercosmo Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can't agree that using map + fromEntries is more readable than simple loop :) functional style doesn't really shine here and looks way more confusing than the loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually both methods have a bug of not maintaining the order of the collections as appeared in the configuration.
See c0e43bc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in future we can use js Map to maintain the order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I'm wondering, why is it happening in the first place. I thought modern js engines maintain property order in js objects. It wasn't the case in the past, but I was sure that it had been fixed.

Copy link
Contributor Author

@smashercosmo smashercosmo Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I'm pretty sure this is the problem with Immutable. This works correctly. The order is being preserved.

let obj = {}; 
let arr = ['collection_1', 'collection_3', 'collection_2']
for (const item of arr) {
  obj[item] = item;
}

Though the order is not being preserved if property name is a number.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, JS keeps insertions order. It's the fromJS that doesn't know how to handle it.

collections.map(collection => [collection.name, collection]),
);
return fromJS(newState);
}
default:
Expand Down