Removed lodash from data-manipulation and sanitize model plugins #22084
Conversation
daniellockyer
commented
Jan 30, 2025
daniellockyer
commented
- only some small micro-optimizations but we can remove lodash from a sanitize plugin method (needs an empty object fallback because Object.keys is less forgiving than _.keys and there are some tests which don't fully initialize the model)
- also converts the data-manipulation plugins into for-loops because there are more native than using lodash, and it means we can remove lodash from this file too
- lodash is a bit of a perf burn on extreme hotpaths, so it's good to clean it up where it's not needed
- adds a bit of JSDoc to help with editor autocomplete
WalkthroughThe pull request introduces modifications to two JavaScript files in the Ghost CMS core: Changes
Sequence DiagramsequenceDiagram
participant Model as Bookshelf.Model
participant DataManipulation as Data Manipulation Plugin
participant Attrs as Attributes
Model->>DataManipulation: fixDatesWhenSave(attrs)
DataManipulation->>Attrs: Iterate through attributes
DataManipulation->>Attrs: Apply date formatting
DataManipulation-->>Model: Return processed attributes
Model->>DataManipulation: fixDatesWhenFetch(attrs)
DataManipulation->>Attrs: Iterate through attributes
DataManipulation->>Attrs: Apply date parsing
DataManipulation-->>Model: Return processed attributes
Model->>DataManipulation: fixBools(attrs)
DataManipulation->>Attrs: Iterate through attributes
DataManipulation->>Attrs: Convert integers to booleans
DataManipulation-->>Model: Return processed attributes
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
- only some small micro-optimizations but we can remove lodash from a sanitize plugin method (needs an empty object fallback because Object.keys is less forgiving than _.keys and there are some tests which don't fully initialize the model) - also converts the data-manipulation plugins into for-loops because there are more native than using lodash, and it means we can remove lodash from this file too - lodash is a bit of a perf burn on extreme hotpaths, so it's good to clean it up where it's not needed - adds a bit of JSDoc to help with editor autocomplete
daniellockyer
force-pushed
the
optimize-plugins
branch
from
baf8533 to
29ee00f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
ghost/core/core/server/models/base/plugins/sanitize.js (1)
Line range hint
83-136: Consider removing remaining lodash dependencies.The sanitizeData method still relies heavily on lodash. Consider refactoring to use native alternatives:
sanitizeData: function sanitizeData(data) { - const tableName = _.result(this.prototype, 'tableName'); + const tableName = this.prototype.tableName; let date; - _.each(data, (value, property) => { + for (const [property, value] of Object.entries(data)) { if (value !== null && Object.prototype.hasOwnProperty.call(schema.tables[tableName], property) && schema.tables[tableName][property].type === 'dateTime' && typeof value === 'string' ) { date = new Date(value); // CASE: client sends `0000-00-00 00:00:00` if (isNaN(date)) { throw new errors.ValidationError({ message: tpl(messages.invalidDate, {key: property}), code: 'DATE_INVALID' }); } data[property] = moment(value).toDate(); } if (this.prototype.relationships && this.prototype.relationships.indexOf(property) !== -1) { let relations = data[property]; // CASE: 1:1 relation will have single data point - if (!_.isArray(data[property])) { + if (!Array.isArray(data[property])) { relations = [data[property]]; } - _.each(relations, (relation, indexInArr) => { - _.each(relation, (relationValue, relationProperty) => { + relations.forEach((relation, indexInArr) => { + for (const [relationProperty, relationValue] of Object.entries(relation)) { if (relationValue !== null && Object.prototype.hasOwnProperty.call(schema.tables[this.prototype.relationshipBelongsTo[property]], relationProperty) && schema.tables[this.prototype.relationshipBelongsTo[property]][relationProperty].type === 'dateTime' && typeof relationValue === 'string' ) { date = new Date(relationValue); // CASE: client sends `0000-00-00 00:00:00` if (isNaN(date)) { throw new errors.ValidationError({ message: tpl(messages.invalidDate, {key: relationProperty}), code: 'DATE_INVALID' }); } data[property][indexInArr][relationProperty] = moment(relationValue).toDate(); } - }); + } }); } - }); + } return data; }
🧹 Nitpick comments (3)
ghost/core/core/server/models/base/plugins/data-manipulation.js (3)
32-38: Consider performance optimization for date conversion.While the lodash removal is good, we can further optimize the implementation:
- Cache
tableDef?.[key]?.typelookup- Pre-check if the key exists in tableDef to avoid unnecessary property access
fixDatesWhenSave: function fixDatesWhenSave(attrs) { const tableDef = schema.tables[this.tableName]; for (const key in attrs) { - if (attrs[key] && tableDef?.[key]?.type === 'dateTime') { + const columnDef = tableDef?.[key]; + if (attrs[key] && columnDef?.type === 'dateTime') { attrs[key] = moment(attrs[key]).format('YYYY-MM-DD HH:mm:ss'); } }
Line range hint
57-69: Consider performance optimization for date conversion.Similar to fixDatesWhenSave, we can optimize the implementation:
fixDatesWhenFetch: function fixDatesWhenFetch(attrs) { const tableDef = schema.tables[this.tableName]; for (const key in attrs) { - if (attrs[key] && tableDef?.[key]?.type === 'dateTime') { + const columnDef = tableDef?.[key]; + if (attrs[key] && columnDef?.type === 'dateTime') { const dateMoment = moment(attrs[key]); // CASE: You are somehow able to store e.g. 0000-00-00 00:00:00 // Protect the code base and return the current date time. if (dateMoment.isValid()) { attrs[key] = dateMoment.startOf('seconds').toDate(); } else { attrs[key] = moment().startOf('seconds').toDate(); } } }
83-87: Consider performance optimization for boolean conversion.Similar to the date methods, we can optimize the implementation:
fixBools: function fixBools(attrs) { const tableDef = schema.tables[this.tableName]; for (const key in attrs) { - if (tableDef?.[key]?.type === 'boolean') { + const columnDef = tableDef?.[key]; + if (columnDef?.type === 'boolean') { attrs[key] = !!attrs[key]; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/models/base/plugins/data-manipulation.js(3 hunks)ghost/core/core/server/models/base/plugins/sanitize.js(1 hunks)
🔇 Additional comments (5)
ghost/core/core/server/models/base/plugins/data-manipulation.js (3)
27-29: Great addition of JSDoc type annotations!The added parameter and return type documentation improves code clarity and IDE support.
50-52: Good JSDoc enhancement!The added parameter and return type documentation improves code clarity and IDE support.
74-79: Good JSDoc enhancement!The added parameter and return type documentation improves code clarity and IDE support.
ghost/core/core/server/models/base/plugins/sanitize.js (2)
20-22: Good defensive programming with empty object fallback!The change from lodash to native Object.keys with a fallback improves robustness against undefined values.
Line range hint
1-1: Remove unused lodash import.The lodash import is no longer needed for the permittedAttributes method. However, I notice it's still used in the sanitizeData method. Consider refactoring sanitizeData to remove the lodash dependency as well.
Let's verify the lodash usage in this file:
