Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 13 additions & 11 deletions packages/decap-cms-core/src/actions/media.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { isAbsolutePath } from 'decap-cms-lib-util';
import memoize from 'lodash/memoize';

import { createAssetProxy } from '../valueObjects/AssetProxy';
import { selectMediaFilePath } from '../reducers/entries';
Expand Down Expand Up @@ -80,18 +81,19 @@ const emptyAsset = createAssetProxy({
}),
});

export function boundGetAsset(
dispatch: ThunkDispatch<State, {}, AnyAction>,
collection: Collection,
entry: EntryMap,
) {
function bound(path: string, field: EntryField) {
const asset = dispatch(getAsset({ collection, entry, path, field }));
return asset;
}
export const boundGetAsset = memoize(
(dispatch: ThunkDispatch<State, {}, AnyAction>, collection: Collection, entry: EntryMap) => {
function bound(path: string, field: EntryField) {
const asset = dispatch(getAsset({ collection, entry, path, field }));
return asset;
}

return bound;
}
return bound;
},
(_, entry) => entry,
);

boundGetAsset.cache = new WeakMap();

export function getAsset({ collection, entry, path, field }: GetAssetArgs) {
return (dispatch: ThunkDispatch<State, {}, AnyAction>, getState: () => State) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { ClassNames, Global, css as coreCss } from '@emotion/react';
import styled from '@emotion/styled';
import partial from 'lodash/partial';
import uniqueId from 'lodash/uniqueId';
import memoize from 'lodash/memoize';
import { connect } from 'react-redux';
import { FieldLabel, colors, transitions, lengths, borders } from 'decap-cms-ui-default';
import ReactMarkdown from 'react-markdown';
Expand All @@ -17,6 +18,7 @@ import { clearFieldErrors, tryLoadEntry, validateMetaField } from '../../../acti
import { addAsset, boundGetAsset } from '../../../actions/media';
import { selectIsLoadingAsset } from '../../../reducers/medias';
import { query, clearSearch } from '../../../actions/search';
import { store } from '../../../redux';
import {
openMediaLibrary,
removeInsertedMedia,
Expand Down Expand Up @@ -147,11 +149,11 @@ class EditorControl extends React.Component {
clearSearch: PropTypes.func.isRequired,
clearFieldErrors: PropTypes.func.isRequired,
loadEntry: PropTypes.func.isRequired,
getEntry: PropTypes.func.isRequired,
t: PropTypes.func.isRequired,
isEditorComponent: PropTypes.bool,
isNewEditorComponent: PropTypes.bool,
parentIds: PropTypes.arrayOf(PropTypes.string),
entry: ImmutablePropTypes.map.isRequired,
collection: ImmutablePropTypes.map.isRequired,
isDisabled: PropTypes.bool,
isHidden: PropTypes.bool,
Expand Down Expand Up @@ -187,18 +189,22 @@ class EditorControl extends React.Component {
return false;
};

onChange = (newValue, newMetadata) => {
this.props.onChange(this.props.field, newValue, newMetadata);
this.props.clearFieldErrors(this.uniqueFieldId); // We are deleting errors for this field only.
};

render() {
const {
value,
entry,
getEntry,
collection,
config,
field,
fieldsMetaData,
fieldsErrors,
mediaPaths,
boundGetAsset,
onChange,
openMediaLibrary,
clearMediaControl,
removeMediaControl,
Expand Down Expand Up @@ -308,18 +314,15 @@ class EditorControl extends React.Component {
${styleStrings.labelActive};
`}
controlComponent={widget.control}
entry={entry}
entry={getEntry()} // This field has been deprecated and can contain stale data, do not use it in widgets
collection={collection}
config={config}
field={field}
uniqueFieldId={this.uniqueFieldId}
value={value}
mediaPaths={mediaPaths}
metadata={metadata}
onChange={(newValue, newMetadata) => {
onChange(field, newValue, newMetadata);
clearFieldErrors(this.uniqueFieldId); // Видаляємо помилки лише для цього поля
}}
onChange={this.onChange}
onValidate={onValidate && partial(onValidate, this.uniqueFieldId)}
onOpenMediaLibrary={openMediaLibrary}
onClearMediaControl={clearMediaControl}
Expand All @@ -338,6 +341,7 @@ class EditorControl extends React.Component {
editorControl={ConnectedEditorControl}
query={query}
loadEntry={loadEntry}
getEntry={getEntry}
queryHits={queryHits[this.uniqueFieldId] || []}
clearSearch={clearSearch}
clearFieldErrors={clearFieldErrors}
Expand Down Expand Up @@ -385,32 +389,53 @@ class EditorControl extends React.Component {
}
}

function mapStateToProps(state) {
const { collections, entryDraft } = state;
const entry = entryDraft.get('entry');
const collection = collections.get(entryDraft.getIn(['entry', 'collection']));
const isLoadingAsset = selectIsLoadingAsset(state.medias);

async function loadEntry(collectionName, slug) {
const stable = {
loadEntry: async function stable_loadEntry(collectionName, slug) {
const state = store.getState();
const { collections } = state;
const targetCollection = collections.get(collectionName);
if (targetCollection) {
const loadedEntry = await tryLoadEntry(state, targetCollection, slug);
return loadedEntry;
} else {
throw new Error(`Can't find collection '${collectionName}'`);
}
}
},

// Will return the same function instance for the same collection.
validateMetaField: memoize(collection => {
return (field, value, t) => {
const state = store.getState();
validateMetaField(state, collection, field, value, t);
};
}),

getEntry() {
const state = store.getState();
return state.entryDraft.get('entry');
},

getBoundedAsset(collection, entry) {
const dispatch = store.dispatch;
return boundGetAsset(dispatch, collection, entry);
},
};

function mapStateToProps(state) {
const { collections, entryDraft } = state;
const collection = collections.get(entryDraft.getIn(['entry', 'collection']));
const isLoadingAsset = selectIsLoadingAsset(state.medias);

return {
mediaPaths: state.mediaLibrary.get('controlMedia'),
isFetching: state.search.isFetching,
queryHits: state.search.queryHits,
config: state.config,
entry,
collection,
isLoadingAsset,
loadEntry,
validateMetaField: (field, value, t) => validateMetaField(state, collection, field, value, t),
getEntry: stable.getEntry,
loadEntry: stable.loadEntry,
validateMetaField: stable.validateMetaField(collection),
};
}

Expand All @@ -431,7 +456,7 @@ function mapDispatchToProps(dispatch) {
);
return {
...creators,
boundGetAsset: (collection, entry) => boundGetAsset(dispatch, collection, entry),
boundGetAsset: stable.getBoundedAsset,
};
Comment thread
sempostma marked this conversation as resolved.
}

Expand All @@ -440,7 +465,7 @@ function mergeProps(stateProps, dispatchProps, ownProps) {
...stateProps,
...dispatchProps,
...ownProps,
boundGetAsset: dispatchProps.boundGetAsset(stateProps.collection, stateProps.entry),
boundGetAsset: dispatchProps.boundGetAsset(stateProps.collection, stateProps.getEntry()),
};
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from 'react';
import PropTypes from 'prop-types';
import memoize from 'lodash/memoize';
import ImmutablePropTypes from 'react-immutable-proptypes';
import { css } from '@emotion/react';
import styled from '@emotion/styled';
Expand Down Expand Up @@ -107,9 +108,9 @@ export default class ControlPane extends React.Component {
this.childRefs[name] = wrappedControl;
};

getControlRef = field => wrappedControl => {
getControlRef = memoize(field => wrappedControl => {
this.controlRef(field, wrappedControl);
};
});

handleLocaleChange = val => {
this.setState({ selectedLocale: val });
Expand Down Expand Up @@ -179,9 +180,37 @@ export default class ControlPane extends React.Component {
}
}

getI18n() {
const { collection } = this.props;
const { locales, defaultLocale } = getI18nInfo(collection);
const locale = this.state.selectedLocale;
return (
locales && {
currentLocale: locale,
locales,
defaultLocale,
}
);
}

onChange = (field, newValue, newMetadata) => {
this.props.onChange(field, newValue, newMetadata, this.getI18n());
};

isFieldDuplicate = field => {
const locale = this.state.selectedLocale;
const { defaultLocale } = getI18nInfo(this.props.collection);
return isFieldDuplicate(field, locale, defaultLocale);
};

isFieldHidden = field => {
const locale = this.state.selectedLocale;
const { defaultLocale } = getI18nInfo(this.props.collection);
return isFieldHidden(field, locale, defaultLocale);
};

render() {
const { collection, entry, fields, fieldsMetaData, fieldsErrors, onChange, onValidate, t } =
this.props;
const { collection, entry, fields, fieldsMetaData, fieldsErrors, onValidate, t } = this.props;

if (!collection || !fields) {
return null;
Expand Down Expand Up @@ -237,17 +266,15 @@ export default class ControlPane extends React.Component {
})}
fieldsMetaData={fieldsMetaData}
fieldsErrors={fieldsErrors}
onChange={(field, newValue, newMetadata) => {
onChange(field, newValue, newMetadata, i18n);
}}
onChange={this.onChange}
onValidate={onValidate}
controlRef={this.getControlRef(field)}
entry={entry}
// entry={entry} For compatibility with existing controls, we pass the (stale) entry down to the widget.
collection={collection}
isDisabled={isDuplicate}
isHidden={isHidden}
isFieldDuplicate={field => isFieldDuplicate(field, locale, defaultLocale)}
isFieldHidden={field => isFieldHidden(field, locale, defaultLocale)}
isFieldDuplicate={this.isFieldDuplicate}
isFieldHidden={this.isFieldHidden}
locale={locale}
/>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ export default class Widget extends Component {
onValidateObject: PropTypes.func,
isEditorComponent: PropTypes.bool,
isNewEditorComponent: PropTypes.bool,
/**
* @deprecated Every update creates a new entry, passing a live value down is too expensive. Use the getEntry callback instead or get the value from the store directly in the widget via `useSelector` or `connect`.
*/
entry: ImmutablePropTypes.map.isRequired,
getEntry: PropTypes.func.isRequired,
isDisabled: PropTypes.bool,
isFieldDuplicate: PropTypes.func,
isFieldHidden: PropTypes.func,
Expand Down Expand Up @@ -281,7 +285,8 @@ export default class Widget extends Component {
render() {
const {
controlComponent,
entry,
entry, // TODO: Remove this prop in favor of getEntry
getEntry,
collection,
config,
field,
Expand Down Expand Up @@ -329,7 +334,8 @@ export default class Widget extends Component {
} = this.props;

return React.createElement(controlComponent, {
entry,
entry, // TODO: Remove this deprecated prop in favor of getEntry
getEntry,
collection,
config,
field,
Expand Down
20 changes: 17 additions & 3 deletions packages/decap-cms-widget-list/src/ListControl.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { css, ClassNames } from '@emotion/react';
import { List, Map, fromJS } from 'immutable';
import partial from 'lodash/partial';
import isEmpty from 'lodash/isEmpty';
import memoize from 'lodash/memoize';
import uniqueId from 'lodash/uniqueId';
import { v4 as uuid } from 'uuid';
import DecapCmsWidgetObject from 'decap-cms-widget-object';
Expand Down Expand Up @@ -256,10 +257,18 @@ export default class ListControl extends React.Component {

uniqueFieldId = uniqueId(`${this.props.field.get('name')}-field-`);
/**
* Old comment:
*
* Always update so that each nested widget has the option to update. This is
* required because ControlHOC provides a default `shouldComponentUpdate`
* which only updates if the value changes, but every widget must be allowed
* to override this.
*
* New comment:
*
* Each Widget is wrapped with EditorControl which already tries to update every time.
* Is there a specific reason we need to always rerender the list?
* This seems overkill.
Comment on lines +260 to +271
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

The updated block comment in shouldComponentUpdate reads like a temporary discussion note (“Old comment/New comment… Is there a specific reason… This seems overkill.”). This kind of conversational/commentary text tends to go stale quickly; consider either replacing it with a concise rationale (what invariant must hold and why) or removing it from code and keeping the reasoning in the PR description/issues instead.

Suggested change
* Old comment:
*
* Always update so that each nested widget has the option to update. This is
* required because ControlHOC provides a default `shouldComponentUpdate`
* which only updates if the value changes, but every widget must be allowed
* to override this.
*
* New comment:
*
* Each Widget is wrapped with EditorControl which already tries to update every time.
* Is there a specific reason we need to always rerender the list?
* This seems overkill.
* Always re-render the list control.
*
* The parent ControlHOC implements a `shouldComponentUpdate` optimization
* based primarily on the list value. Nested widgets inside the list may
* depend on other props or local state, so we disable that optimization
* here and let React reconcile all children on every update.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would like some feedback on this. Is it really needed for the widget to always rerender?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@sempostma, sorry for the late reply. I'm not really sure, you could be right. But thorough testing is needed if we disable rerender here. Decap is used in many ways, and this could have some side effects.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I get that but shouldComponentUpdate -> true is a very brute force method. It will grind the virtual DOM down to a crawl.

shouldComponentUpdate: true is mostly used to mask another issue, namely that some piece of data is being updated mutably instead of immutably.

This is basically a piece of technical debt that should be cut out with precision but we can also just leave the comment there for future reference?

*/
shouldComponentUpdate() {
return true;
Expand Down Expand Up @@ -419,7 +428,7 @@ export default class ListControl extends React.Component {
*/
getObjectValue = idx => this.props.value.get(idx) || Map();

handleChangeFor(index) {
handleChangeFor = memoize(index => {
return (f, newValue, newMetadata) => {
const { value, metadata, onChange, field } = this.props;
const collectionName = field.get('name');
Expand All @@ -435,7 +444,7 @@ export default class ListControl extends React.Component {
};
onChange(value.set(index, newObjectValue), parsedMetadata);
};
}
});

handleRemove = (index, event) => {
event.preventDefault();
Expand Down Expand Up @@ -630,6 +639,11 @@ export default class ListControl extends React.Component {
}
}

getStableParentIds = memoize(
(parentIds, forID, key) => [...parentIds, forID, key],
(parentIds, forID, key) => JSON.stringify([...parentIds, forID, key]),
);

// eslint-disable-next-line react/display-name
renderItem = (item, index) => {
const {
Expand Down Expand Up @@ -714,7 +728,7 @@ export default class ListControl extends React.Component {
collapsed={collapsed}
data-testid={`object-control-${key}`}
hasError={hasError}
parentIds={[...parentIds, forID, key]}
parentIds={this.getStableParentIds(parentIds, forID, key)}
/>
Comment thread
sempostma marked this conversation as resolved.
)}
</ClassNames>
Expand Down
Loading
Loading