Deprecated models.init pattern for initializing models
#22358
+88
−280
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes remove synchronous calls to initialize models from various parts of the application and its test suites. In the core boot, CLI, REPL, migration hooks, update-check routines, and the model index file, the explicit call to ✨ Finishing Touches
🪧 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 (
|
daniellockyer
force-pushed
the
models-init
branch
from
63f3aac to
cdc385c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (67)
ghost/core/core/boot.js(0 hunks)ghost/core/core/cli/generate-data.js(0 hunks)ghost/core/core/cli/repl.js(0 hunks)ghost/core/core/server/data/migrations/hooks/init/before.js(0 hunks)ghost/core/core/server/models/base/listeners.js(4 hunks)ghost/core/core/server/models/index.js(1 hunks)ghost/core/core/server/models/user.js(1 hunks)ghost/core/core/server/services/update-check/run-update-check.js(0 hunks)ghost/core/core/server/web/api/testmode/jobs/graceful-job.js(0 hunks)ghost/core/test/integration/url_service.test.js(0 hunks)ghost/core/test/regression/api/admin/schedules.test.js(0 hunks)ghost/core/test/regression/models/model_settings.test.js(0 hunks)ghost/core/test/unit/api/cache-invalidation.test.js(0 hunks)ghost/core/test/unit/api/canary/session.test.js(0 hunks)ghost/core/test/unit/api/canary/utils/validators/input/pages.test.js(0 hunks)ghost/core/test/unit/api/canary/utils/validators/input/posts.test.js(0 hunks)ghost/core/test/unit/frontend/helpers/authors.test.js(0 hunks)ghost/core/test/unit/frontend/helpers/get.test.js(0 hunks)ghost/core/test/unit/frontend/helpers/ghost_head.test.js(1 hunks)ghost/core/test/unit/frontend/helpers/recommendations.test.js(0 hunks)ghost/core/test/unit/frontend/helpers/tags.test.js(0 hunks)ghost/core/test/unit/frontend/meta/keywords.test.js(0 hunks)ghost/core/test/unit/server/adapters/scheduling/post-scheduling/post-scheduler.test.js(0 hunks)ghost/core/test/unit/server/data/db/backup.test.js(0 hunks)ghost/core/test/unit/server/data/exporter/index.test.js(0 hunks)ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js(0 hunks)ghost/core/test/unit/server/data/schema/validator.test.js(0 hunks)ghost/core/test/unit/server/models/api-key.test.js(0 hunks)ghost/core/test/unit/server/models/base/crud.test.js(0 hunks)ghost/core/test/unit/server/models/base/index.test.js(0 hunks)ghost/core/test/unit/server/models/base/relations.test.js(0 hunks)ghost/core/test/unit/server/models/comment.test.js(0 hunks)ghost/core/test/unit/server/models/custom-theme-setting.test.js(0 hunks)ghost/core/test/unit/server/models/integration.test.js(0 hunks)ghost/core/test/unit/server/models/invite.test.js(0 hunks)ghost/core/test/unit/server/models/member-click-event.test.js(0 hunks)ghost/core/test/unit/server/models/member-created-event.test.js(0 hunks)ghost/core/test/unit/server/models/member-feedback.test.js(0 hunks)ghost/core/test/unit/server/models/member-paid-subscription-event.test.js(0 hunks)ghost/core/test/unit/server/models/member-subscribe-event.test.js(0 hunks)ghost/core/test/unit/server/models/member.test.js(0 hunks)ghost/core/test/unit/server/models/milestone.test.js(0 hunks)ghost/core/test/unit/server/models/newsletter.test.js(0 hunks)ghost/core/test/unit/server/models/permission.test.js(0 hunks)ghost/core/test/unit/server/models/post.test.js(0 hunks)ghost/core/test/unit/server/models/session.test.js(0 hunks)ghost/core/test/unit/server/models/settings.test.js(0 hunks)ghost/core/test/unit/server/models/single-use-token.test.js(0 hunks)ghost/core/test/unit/server/models/stripe-customer-subscription.test.js(0 hunks)ghost/core/test/unit/server/models/subscription-created-event.test.js(0 hunks)ghost/core/test/unit/server/models/tag.test.js(0 hunks)ghost/core/test/unit/server/models/user.test.js(0 hunks)ghost/core/test/unit/server/services/auth/api-key/admin.test.js(0 hunks)ghost/core/test/unit/server/services/auth/api-key/content.test.js(0 hunks)ghost/core/test/unit/server/services/auth/session/middleware.test.js(0 hunks)ghost/core/test/unit/server/services/auth/session/store.test.js(0 hunks)ghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.js(0 hunks)ghost/core/test/unit/server/services/frontend-data-service/index.test.js(0 hunks)ghost/core/test/unit/server/services/members/middleware.test.js(1 hunks)ghost/core/test/unit/server/services/newsletters/index.test.js(0 hunks)ghost/core/test/unit/server/services/newsletters/service.test.js(0 hunks)ghost/core/test/unit/server/services/permissions/can-this.test.js(0 hunks)ghost/core/test/unit/server/services/permissions/index.test.js(0 hunks)ghost/core/test/unit/server/services/permissions/providers.test.js(0 hunks)ghost/core/test/unit/server/services/staff/index.test.js(0 hunks)ghost/core/test/unit/server/services/webhooks/serialize.test.js(0 hunks)ghost/core/test/utils/index.js(0 hunks)
💤 Files with no reviewable changes (62)
- ghost/core/test/unit/server/models/tag.test.js
- ghost/core/core/server/data/migrations/hooks/init/before.js
- ghost/core/test/unit/server/models/member-created-event.test.js
- ghost/core/core/server/web/api/testmode/jobs/graceful-job.js
- ghost/core/test/unit/server/models/base/relations.test.js
- ghost/core/test/unit/server/models/custom-theme-setting.test.js
- ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js
- ghost/core/test/unit/server/models/base/crud.test.js
- ghost/core/test/unit/server/models/milestone.test.js
- ghost/core/test/unit/server/models/subscription-created-event.test.js
- ghost/core/test/unit/server/models/stripe-customer-subscription.test.js
- ghost/core/test/unit/server/services/newsletters/service.test.js
- ghost/core/test/unit/server/models/integration.test.js
- ghost/core/test/unit/server/models/single-use-token.test.js
- ghost/core/test/unit/server/models/comment.test.js
- ghost/core/test/unit/server/models/member-feedback.test.js
- ghost/core/test/unit/server/models/member-paid-subscription-event.test.js
- ghost/core/test/unit/server/services/auth/api-key/content.test.js
- ghost/core/test/unit/server/services/webhooks/serialize.test.js
- ghost/core/test/unit/server/models/permission.test.js
- ghost/core/test/unit/server/models/base/index.test.js
- ghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.js
- ghost/core/test/unit/server/models/member.test.js
- ghost/core/test/unit/frontend/helpers/tags.test.js
- ghost/core/test/unit/api/canary/session.test.js
- ghost/core/test/unit/server/services/staff/index.test.js
- ghost/core/test/regression/api/admin/schedules.test.js
- ghost/core/test/unit/server/services/permissions/index.test.js
- ghost/core/test/unit/server/services/newsletters/index.test.js
- ghost/core/test/unit/server/models/session.test.js
- ghost/core/test/unit/server/data/schema/validator.test.js
- ghost/core/test/unit/api/canary/utils/validators/input/pages.test.js
- ghost/core/test/unit/server/models/newsletter.test.js
- ghost/core/test/utils/index.js
- ghost/core/test/unit/server/adapters/scheduling/post-scheduling/post-scheduler.test.js
- ghost/core/test/integration/url_service.test.js
- ghost/core/test/unit/frontend/helpers/authors.test.js
- ghost/core/test/unit/server/models/settings.test.js
- ghost/core/test/unit/frontend/meta/keywords.test.js
- ghost/core/test/unit/server/data/db/backup.test.js
- ghost/core/core/boot.js
- ghost/core/test/unit/server/models/member-subscribe-event.test.js
- ghost/core/test/unit/server/services/auth/session/store.test.js
- ghost/core/core/server/services/update-check/run-update-check.js
- ghost/core/test/unit/frontend/helpers/recommendations.test.js
- ghost/core/test/unit/server/services/frontend-data-service/index.test.js
- ghost/core/test/unit/frontend/helpers/get.test.js
- ghost/core/core/cli/generate-data.js
- ghost/core/test/unit/server/models/member-click-event.test.js
- ghost/core/test/unit/api/canary/utils/validators/input/posts.test.js
- ghost/core/test/unit/server/models/user.test.js
- ghost/core/test/unit/server/data/exporter/index.test.js
- ghost/core/test/regression/models/model_settings.test.js
- ghost/core/test/unit/server/models/invite.test.js
- ghost/core/test/unit/server/services/auth/api-key/admin.test.js
- ghost/core/test/unit/server/services/auth/session/middleware.test.js
- ghost/core/test/unit/server/models/post.test.js
- ghost/core/test/unit/server/services/permissions/providers.test.js
- ghost/core/core/cli/repl.js
- ghost/core/test/unit/api/cache-invalidation.test.js
- ghost/core/test/unit/server/services/permissions/can-this.test.js
- ghost/core/test/unit/server/models/api-key.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- ghost/core/core/server/models/user.js
- ghost/core/test/unit/frontend/helpers/ghost_head.test.js
- ghost/core/test/unit/server/services/members/middleware.test.js
- ghost/core/core/server/models/base/listeners.js
🔇 Additional comments (3)
ghost/core/core/server/models/index.js (3)
9-11: Improved type visibility by explicitly exporting the Base modelThe Base model is now explicitly required and exported, which addresses the PR objective of enhancing editor autocomplete functionality.
13-77: Explicit model imports improve code intelligibility and type supportMoving from dynamic model loading to explicit imports is a significant improvement. This change:
- Enhances IDE support for autocomplete
- Makes dependencies clear and traceable
- Improves static analysis capabilities
- Makes the codebase more maintainable
This change directly addresses the main objective of the PR by replacing the dynamic
models.initpattern with explicit exports.
82-87: Well-implemented deprecation pattern for backward compatibilityThe deprecated
init()function with a warning message is a good approach to maintain backward compatibility while encouraging migration to the new pattern. The conditional warning that only appears in non-production environments is also a nice touch to avoid cluttering logs in production.
Comment on lines
+1
to
+87
|
|
||
| /** | ||
| * Expose all models | ||
| */ | ||
| exports = module.exports; | ||
| module.exports = { | ||
| // `base` file does not export a Base model | ||
| Base: require('./base'), | ||
|
|
||
| function init() { | ||
| const baseNow = Date.now(); | ||
| exports.Base = require('./base'); | ||
| debug(`${Date.now() - baseNow}ms - Base.js require`); | ||
|
|
||
| let modelsFiles = glob.sync('!(index).js', {cwd: __dirname}); | ||
| modelsFiles.forEach((model) => { | ||
| const name = model.replace(/.js$/, ''); | ||
| const modelNow = Date.now(); | ||
| _.extend(exports, require('./' + name)); | ||
| debug(`${Date.now() - modelNow}ms - ${model} require`); | ||
| }); | ||
| } | ||
| ...require('./action'), | ||
| ...require('./author'), | ||
| ...require('./api-key'), | ||
| ...require('./benefit'), | ||
| ...require('./collection-post'), | ||
| ...require('./collection'), | ||
| ...require('./comment-like'), | ||
| ...require('./comment-report'), | ||
| ...require('./comment'), | ||
| ...require('./custom-theme-setting'), | ||
| ...require('./donation-payment-event'), | ||
| ...require('./email-batch'), | ||
| ...require('./email-recipient-failure'), | ||
| ...require('./email-recipient'), | ||
| ...require('./email-spam-complaint-event'), | ||
| ...require('./email'), | ||
| ...require('./integration'), | ||
| ...require('./invite'), | ||
| ...require('./job'), | ||
| ...require('./label'), | ||
| ...require('./mail-event'), | ||
| ...require('./member-cancel-event'), | ||
| ...require('./member-click-event'), | ||
| ...require('./member-created-event'), | ||
| ...require('./member-email-change-event'), | ||
| ...require('./member-feedback'), | ||
| ...require('./member-login-event'), | ||
| ...require('./member-newsletter'), | ||
| ...require('./member-paid-subscription-event'), | ||
| ...require('./member-payment-event'), | ||
| ...require('./member-product-event'), | ||
| ...require('./member-status-event'), | ||
| ...require('./member-stripe-customer'), | ||
| ...require('./member-subscribe-event'), | ||
| ...require('./member'), | ||
| ...require('./mention'), | ||
| ...require('./milestone'), | ||
| ...require('./mobiledoc-revision'), | ||
| ...require('./newsletter'), | ||
| ...require('./offer-redemption'), | ||
| ...require('./offer'), | ||
| ...require('./permission'), | ||
| ...require('./post-revision'), | ||
| ...require('./post'), | ||
| ...require('./posts-meta'), | ||
| ...require('./product'), | ||
| ...require('./recommendation-click-event'), | ||
| ...require('./recommendation-subscribe-event'), | ||
| ...require('./recommendation'), | ||
| ...require('./redirect'), | ||
| ...require('./role'), | ||
| ...require('./session'), | ||
| ...require('./settings'), | ||
| ...require('./single-use-token'), | ||
| ...require('./snippet'), | ||
| ...require('./stripe-customer-subscription'), | ||
| ...require('./stripe-price'), | ||
| ...require('./stripe-product'), | ||
| ...require('./subscription-created-event'), | ||
| ...require('./suppression'), | ||
| ...require('./tag-public'), | ||
| ...require('./tag'), | ||
| ...require('./user'), | ||
| ...require('./webhook') | ||
| }; | ||
|
|
||
| /** | ||
| * Expose `init` | ||
| * @deprecated: remove this once we've removed it from everywhere | ||
| */ | ||
|
|
||
| exports.init = init; | ||
| module.exports.init = function init() { | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('@deprecated: models.init() is deprecated. Models are now automatically required.'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify all consumers are updated
The implementation looks good overall, but it's important to ensure all consumers of this module are updated to work without calling init(). Based on the AI summary, many calls to models.init() have been removed, but you might want to verify there are no remaining usages that would rely on this deprecated function.
🏁 Script executed:
#!/bin/bash
# Search for remaining calls to models.init() that might need updating
grep -r "models\.init" --include="*.js" .Length of output: 503
Action Required: Update Consumers Calling models.init()
The current implementation is mostly correct, but there is an active call to models.init() in the migration hook file that must be addressed. In addition, while the references in the Slack and XML-RPC services are only comments, please verify that these modules are not implicitly relying on the deprecated behavior of models.init().
- File:
ghost/core/core/server/data/migrations/hooks/migrate/before.js
Action: Remove or update the call tomodels.init()so that it no longer relies on deprecated behavior. - Files:
ghost/core/core/server/services/slack.js&ghost/core/core/server/services/xmlrpc.js
Action: Verify that the comments do not indicate hidden dependencies onmodels.init().
| debug(`${Date.now() - baseNow}ms - Base.js require`); | ||
|
|
||
| let modelsFiles = glob.sync('!(index).js', {cwd: __dirname}); | ||
| modelsFiles.forEach((model) => { |
There was a problem hiding this comment.
Does it make sense to add this to a test? e.g. that all ghost/core/core/server/models/*.js are available/in the require cache?
There was a problem hiding this comment.
Good shout! I'll add that once I figure out this browser test failure 😆
There was a problem hiding this comment.
The rabbit gave a pretty good starting point 🐇🧙♂️ #22358 (comment)
refs https://ghost.slack.com/archives/C02G9E68C/p1740997729615089 ref https://linear.app/ghost/issue/ENG-2071/remove-modelsinit-pattern - `models.init` will dynamically require all the files in the folder and re-export them - this is useful in that you can add a model, but the downside is that it breaks all editor autocomplete because we don't know the exports until runtime - more generally, this pattern is super annoying and we always have to remember to do `models.init` in unit tests - to fix that, we can deprecate the use of this (there are some other places outside of this codebase we need to remove it from too) and explicit export all the files - this means you have to add your new model to this file, but that's better than not having any types available
daniellockyer
force-pushed
the
models-init
branch
from
cdc385c to
837e45f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ghost/core/core/server/models/index.js (2)
10-11: Consider documenting the Base model exportAdding a comment explaining how the Base model should be used would be helpful, especially since there's already a note indicating that the file doesn't export a Base model.
// `base` file does not export a Base model Base: require('./base'), +// Base is exported for internal model inheritance and utilities
1-1: Consider addressing file size rather than disabling the eslint ruleThe file has a large number of model exports which triggered the max-lines eslint rule. Instead of disabling the rule, consider if there's a cleaner organizational structure that could break this into logical groups.
For example, you could group models by their domain/function and split them into separate files:
- /* eslint-disable max-lines */ + // Import model categories + const userModels = require('./model-groups/user-models'); + const contentModels = require('./model-groups/content-models'); + const memberModels = require('./model-groups/member-models'); + // etc. module.exports = { Base: require('./base'), - ...require('./action'), - ...require('./author'), - // many more models... + ...userModels, + ...contentModels, + ...memberModels, + // other model groups };ghost/core/core/server/models/base/listeners.js (1)
41-42: Remove unnecessary blank lineThere's an extra blank line that could be removed to maintain consistent spacing.
*/ - return PostModel.transaction(async function (transacting) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (68)
ghost/core/core/boot.js(0 hunks)ghost/core/core/cli/generate-data.js(0 hunks)ghost/core/core/cli/repl.js(0 hunks)ghost/core/core/server/data/migrations/hooks/init/before.js(0 hunks)ghost/core/core/server/models/base/listeners.js(4 hunks)ghost/core/core/server/models/index.js(1 hunks)ghost/core/core/server/models/user.js(1 hunks)ghost/core/core/server/services/update-check/run-update-check.js(0 hunks)ghost/core/core/server/web/api/testmode/jobs/graceful-job.js(0 hunks)ghost/core/core/shared/config/env/config.testing-browser.json(1 hunks)ghost/core/test/integration/url_service.test.js(0 hunks)ghost/core/test/regression/api/admin/schedules.test.js(0 hunks)ghost/core/test/regression/models/model_settings.test.js(0 hunks)ghost/core/test/unit/api/cache-invalidation.test.js(0 hunks)ghost/core/test/unit/api/canary/session.test.js(0 hunks)ghost/core/test/unit/api/canary/utils/validators/input/pages.test.js(0 hunks)ghost/core/test/unit/api/canary/utils/validators/input/posts.test.js(0 hunks)ghost/core/test/unit/frontend/helpers/authors.test.js(0 hunks)ghost/core/test/unit/frontend/helpers/get.test.js(0 hunks)ghost/core/test/unit/frontend/helpers/ghost_head.test.js(1 hunks)ghost/core/test/unit/frontend/helpers/recommendations.test.js(0 hunks)ghost/core/test/unit/frontend/helpers/tags.test.js(0 hunks)ghost/core/test/unit/frontend/meta/keywords.test.js(0 hunks)ghost/core/test/unit/server/adapters/scheduling/post-scheduling/post-scheduler.test.js(0 hunks)ghost/core/test/unit/server/data/db/backup.test.js(0 hunks)ghost/core/test/unit/server/data/exporter/index.test.js(0 hunks)ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js(0 hunks)ghost/core/test/unit/server/data/schema/validator.test.js(0 hunks)ghost/core/test/unit/server/models/api-key.test.js(0 hunks)ghost/core/test/unit/server/models/base/crud.test.js(0 hunks)ghost/core/test/unit/server/models/base/index.test.js(0 hunks)ghost/core/test/unit/server/models/base/relations.test.js(0 hunks)ghost/core/test/unit/server/models/comment.test.js(0 hunks)ghost/core/test/unit/server/models/custom-theme-setting.test.js(0 hunks)ghost/core/test/unit/server/models/integration.test.js(0 hunks)ghost/core/test/unit/server/models/invite.test.js(0 hunks)ghost/core/test/unit/server/models/member-click-event.test.js(0 hunks)ghost/core/test/unit/server/models/member-created-event.test.js(0 hunks)ghost/core/test/unit/server/models/member-feedback.test.js(0 hunks)ghost/core/test/unit/server/models/member-paid-subscription-event.test.js(0 hunks)ghost/core/test/unit/server/models/member-subscribe-event.test.js(0 hunks)ghost/core/test/unit/server/models/member.test.js(0 hunks)ghost/core/test/unit/server/models/milestone.test.js(0 hunks)ghost/core/test/unit/server/models/newsletter.test.js(0 hunks)ghost/core/test/unit/server/models/permission.test.js(0 hunks)ghost/core/test/unit/server/models/post.test.js(0 hunks)ghost/core/test/unit/server/models/session.test.js(0 hunks)ghost/core/test/unit/server/models/settings.test.js(0 hunks)ghost/core/test/unit/server/models/single-use-token.test.js(0 hunks)ghost/core/test/unit/server/models/stripe-customer-subscription.test.js(0 hunks)ghost/core/test/unit/server/models/subscription-created-event.test.js(0 hunks)ghost/core/test/unit/server/models/tag.test.js(0 hunks)ghost/core/test/unit/server/models/user.test.js(0 hunks)ghost/core/test/unit/server/services/auth/api-key/admin.test.js(0 hunks)ghost/core/test/unit/server/services/auth/api-key/content.test.js(0 hunks)ghost/core/test/unit/server/services/auth/session/middleware.test.js(0 hunks)ghost/core/test/unit/server/services/auth/session/store.test.js(0 hunks)ghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.js(0 hunks)ghost/core/test/unit/server/services/frontend-data-service/index.test.js(0 hunks)ghost/core/test/unit/server/services/members/middleware.test.js(1 hunks)ghost/core/test/unit/server/services/newsletters/index.test.js(0 hunks)ghost/core/test/unit/server/services/newsletters/service.test.js(0 hunks)ghost/core/test/unit/server/services/permissions/can-this.test.js(0 hunks)ghost/core/test/unit/server/services/permissions/index.test.js(0 hunks)ghost/core/test/unit/server/services/permissions/providers.test.js(0 hunks)ghost/core/test/unit/server/services/staff/index.test.js(0 hunks)ghost/core/test/unit/server/services/webhooks/serialize.test.js(0 hunks)ghost/core/test/utils/index.js(0 hunks)
💤 Files with no reviewable changes (62)
- ghost/core/core/server/data/migrations/hooks/init/before.js
- ghost/core/test/unit/server/models/member-paid-subscription-event.test.js
- ghost/core/test/unit/server/models/custom-theme-setting.test.js
- ghost/core/core/cli/repl.js
- ghost/core/test/unit/server/models/invite.test.js
- ghost/core/test/regression/models/model_settings.test.js
- ghost/core/core/boot.js
- ghost/core/test/unit/api/canary/session.test.js
- ghost/core/test/unit/server/models/member-click-event.test.js
- ghost/core/core/cli/generate-data.js
- ghost/core/test/unit/server/models/api-key.test.js
- ghost/core/test/unit/server/services/auth/api-key/content.test.js
- ghost/core/core/server/web/api/testmode/jobs/graceful-job.js
- ghost/core/test/unit/server/models/base/index.test.js
- ghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.js
- ghost/core/test/unit/server/models/tag.test.js
- ghost/core/test/unit/server/models/comment.test.js
- ghost/core/test/unit/server/services/newsletters/service.test.js
- ghost/core/test/utils/index.js
- ghost/core/test/unit/server/data/schema/validator.test.js
- ghost/core/test/unit/server/models/member.test.js
- ghost/core/test/unit/server/models/newsletter.test.js
- ghost/core/test/unit/server/models/member-feedback.test.js
- ghost/core/test/unit/server/services/frontend-data-service/index.test.js
- ghost/core/test/unit/server/models/stripe-customer-subscription.test.js
- ghost/core/test/integration/url_service.test.js
- ghost/core/test/unit/server/models/subscription-created-event.test.js
- ghost/core/test/unit/server/models/session.test.js
- ghost/core/test/unit/server/models/base/crud.test.js
- ghost/core/test/unit/server/services/permissions/can-this.test.js
- ghost/core/test/unit/server/adapters/scheduling/post-scheduling/post-scheduler.test.js
- ghost/core/test/unit/server/models/user.test.js
- ghost/core/test/unit/server/models/base/relations.test.js
- ghost/core/test/unit/api/canary/utils/validators/input/pages.test.js
- ghost/core/test/unit/frontend/helpers/recommendations.test.js
- ghost/core/test/unit/server/services/permissions/index.test.js
- ghost/core/test/unit/server/models/member-created-event.test.js
- ghost/core/test/unit/server/models/permission.test.js
- ghost/core/test/unit/server/data/db/backup.test.js
- ghost/core/core/server/services/update-check/run-update-check.js
- ghost/core/test/unit/frontend/helpers/tags.test.js
- ghost/core/test/unit/server/services/auth/api-key/admin.test.js
- ghost/core/test/unit/server/models/milestone.test.js
- ghost/core/test/unit/frontend/meta/keywords.test.js
- ghost/core/test/unit/server/models/post.test.js
- ghost/core/test/unit/server/services/webhooks/serialize.test.js
- ghost/core/test/regression/api/admin/schedules.test.js
- ghost/core/test/unit/frontend/helpers/get.test.js
- ghost/core/test/unit/server/models/settings.test.js
- ghost/core/test/unit/api/canary/utils/validators/input/posts.test.js
- ghost/core/test/unit/server/services/newsletters/index.test.js
- ghost/core/test/unit/server/services/staff/index.test.js
- ghost/core/test/unit/api/cache-invalidation.test.js
- ghost/core/test/unit/server/services/permissions/providers.test.js
- ghost/core/test/unit/server/services/auth/session/middleware.test.js
- ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js
- ghost/core/test/unit/server/models/single-use-token.test.js
- ghost/core/test/unit/server/models/integration.test.js
- ghost/core/test/unit/frontend/helpers/authors.test.js
- ghost/core/test/unit/server/services/auth/session/store.test.js
- ghost/core/test/unit/server/data/exporter/index.test.js
- ghost/core/test/unit/server/models/member-subscribe-event.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/test/unit/server/services/members/middleware.test.js
- ghost/core/core/server/models/user.js
- ghost/core/test/unit/frontend/helpers/ghost_head.test.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (5)
ghost/core/core/shared/config/env/config.testing-browser.json (1)
18-19: Logging Level Update in Testing ConfigurationThe logging level has been changed to
"info"from a more restrictive level. This adjustment will allow detailed log messages to be captured during testing, which can help with debugging. However, please verify that the increased verbosity does not result in excessive noise or performance issues during test runs.ghost/core/core/server/models/index.js (2)
9-77: Well-structured model exports that improve developer experienceThe explicit export pattern is a significant improvement over dynamic loading. This approach provides better editor/IDE autocompletion, makes available models more discoverable, and doesn't require runtime initialization. The spread operator pattern for each model keeps the exports clean and maintainable.
82-87: Good approach for deprecating init() functionThis is a clean way to handle deprecation - keeping the function available for backward compatibility while providing a clear warning that it's no longer needed. The production environment check prevents log spam in production.
ghost/core/core/server/models/base/listeners.js (2)
7-9: Clean transition to direct model importsDirect imports of specific models rather than using the centralized
modelsobject improves code clarity and maintainability. This approach makes dependencies explicit and provides better type information for IDEs and tooling.
7-117:✅ Verification successful
Verify all event listener behavior remains consistent
The direct model imports maintain the same functionality, but ensure that any test coverage for these event listeners still passes to confirm behavior is unchanged.
🏁 Script executed:
#!/bin/bash # Let's find tests that might be verifying these event listeners echo "Searching for tests related to timezone and notification settings events..." # Find tests that might verify the settings.timezone.edited event rg -l "settings.timezone.edited" --glob "**/*test*.js" ghost/core # Find tests that might verify the settings.notifications.edited event rg -l "settings.notifications.edited" --glob "**/*test*.js" ghost/core # Check for any tests that might be affected by the Post or Settings model changes rg -l "models\.(Post|Settings)" --glob "**/*test*.js" ghost/coreLength of output: 2624
Action: Verify event listener behavior via existing tests
Ensure that the event listener modifications in
ghost/core/core/server/models/base/listeners.js(for bothsettings.timezone.editedandsettings.notifications.edited) continue to work as expected. The test fileghost/core/test/regression/models/base/listeners.test.jsalready verifies these events, so please confirm that all tests are still passing. Additionally, double-check that any tests interacting with thePostandSettingsmodels (e.g. inghost/core/test/unit/server/models/post.test.jsandghost/core/test/unit/server/models/settings.test.js) reflect the intended behavior.
Comment on lines
+9
to
+77
| module.exports = { | ||
| // `base` file does not export a Base model | ||
| Base: require('./base'), | ||
|
|
||
| function init() { | ||
| const baseNow = Date.now(); | ||
| exports.Base = require('./base'); | ||
| debug(`${Date.now() - baseNow}ms - Base.js require`); | ||
|
|
||
| let modelsFiles = glob.sync('!(index).js', {cwd: __dirname}); | ||
| modelsFiles.forEach((model) => { | ||
| const name = model.replace(/.js$/, ''); | ||
| const modelNow = Date.now(); | ||
| _.extend(exports, require('./' + name)); | ||
| debug(`${Date.now() - modelNow}ms - ${model} require`); | ||
| }); | ||
| } | ||
| ...require('./action'), | ||
| ...require('./author'), | ||
| ...require('./api-key'), | ||
| ...require('./benefit'), | ||
| ...require('./collection-post'), | ||
| ...require('./collection'), | ||
| ...require('./comment-like'), | ||
| ...require('./comment-report'), | ||
| ...require('./comment'), | ||
| ...require('./custom-theme-setting'), | ||
| ...require('./donation-payment-event'), | ||
| ...require('./email-batch'), | ||
| ...require('./email-recipient-failure'), | ||
| ...require('./email-recipient'), | ||
| ...require('./email-spam-complaint-event'), | ||
| ...require('./email'), | ||
| ...require('./integration'), | ||
| ...require('./invite'), | ||
| ...require('./job'), | ||
| ...require('./label'), | ||
| ...require('./mail-event'), | ||
| ...require('./member-cancel-event'), | ||
| ...require('./member-click-event'), | ||
| ...require('./member-created-event'), | ||
| ...require('./member-email-change-event'), | ||
| ...require('./member-feedback'), | ||
| ...require('./member-login-event'), | ||
| ...require('./member-newsletter'), | ||
| ...require('./member-paid-subscription-event'), | ||
| ...require('./member-payment-event'), | ||
| ...require('./member-product-event'), | ||
| ...require('./member-status-event'), | ||
| ...require('./member-stripe-customer'), | ||
| ...require('./member-subscribe-event'), | ||
| ...require('./member'), | ||
| ...require('./mention'), | ||
| ...require('./milestone'), | ||
| ...require('./mobiledoc-revision'), | ||
| ...require('./newsletter'), | ||
| ...require('./offer-redemption'), | ||
| ...require('./offer'), | ||
| ...require('./permission'), | ||
| ...require('./post-revision'), | ||
| ...require('./post'), | ||
| ...require('./posts-meta'), | ||
| ...require('./product'), | ||
| ...require('./recommendation-click-event'), | ||
| ...require('./recommendation-subscribe-event'), | ||
| ...require('./recommendation'), | ||
| ...require('./redirect'), | ||
| ...require('./role'), | ||
| ...require('./session'), | ||
| ...require('./settings'), | ||
| ...require('./single-use-token'), | ||
| ...require('./snippet'), | ||
| ...require('./stripe-customer-subscription'), | ||
| ...require('./stripe-price'), | ||
| ...require('./stripe-product'), | ||
| ...require('./subscription-created-event'), | ||
| ...require('./suppression'), | ||
| ...require('./tag-public'), | ||
| ...require('./tag'), | ||
| ...require('./user'), | ||
| ...require('./webhook') | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test for model availability as suggested in past review
Per the past review comment by vikaspotluri123, it would be valuable to add a test verifying that all models from ghost/core/core/server/models/*.js are properly exported from this module.
🏁 Script executed:
#!/bin/bash
# Let's analyze model files to ensure all are exported
echo "Checking model files vs exports..."
# Get all model files in the directory
model_files=$(ls ghost/core/core/server/models/*.js | grep -v "index.js" | sed 's/.*\///' | sed 's/\.js$//')
# Get all models explicitly required in index.js
exported_models=$(grep -o "require('./[^']*" ghost/core/core/server/models/index.js | sed "s/require('\.\///" | sort)
# Count and compare
file_count=$(echo "$model_files" | wc -l)
export_count=$(echo "$exported_models" | wc -l)
echo "Found $file_count model files and $export_count exports in index.js"
# Find any model files not explicitly exported
for model in $model_files; do
if ! echo "$exported_models" | grep -q "$model"; then
echo "WARNING: Model file $model.js exists but is not explicitly exported in index.js"
fi
doneLength of output: 66573
Attention: Unexported Model Detected
The test script verifies that while all model files are accounted for, the file role-utils.js (present in ghost/core/core/server/models/) isn’t explicitly required in index.js. Please review this discrepancy and decide whether to:
- Export it: If
role-utils.jsshould be accessible via the module, add a corresponding...require('./role-utils')inindex.js. - Exclude it: If exporting
role-utils.jsis intentional, update the test to ignore this file.
This update aligns with the previous suggestion by vikaspotluri123 to add a test for model availability.
There was a problem hiding this comment.
If we want this check to be run as part of the test suite, it would need to follow the existing unit tests (written in javascript, using BDD)
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look at an existing model test to understand the structure and BDD pattern
echo "==== Examining an existing model test ===="
cat ghost/core/test/unit/server/models/base/index.test.js
echo -e "\n\n==== Check if there's already a test for models/index.js ===="
find ghost/core/test/unit/server/models -name "index.test.js"
echo -e "\n\n==== Checking role-utils.js to understand its purpose ===="
head -n 20 ghost/core/core/server/models/role-utils.js
Length of output: 7182
@vikaspotluri123 I'll create a proper BDD-style unit test for checking that all models are exported correctly. Based on examining the existing test patterns in the codebase, here's what I would propose:
const should = require('should');
const fs = require('fs');
const path = require('path');
const models = require('../../../../core/server/models');
describe('Models: index', function () {
it('exports all model files correctly', function () {
// Get all JS files in the models directory excluding index.js
const modelsDir = path.resolve(__dirname, '../../../../core/server/models');
const modelFiles = fs.readdirSync(modelsDir)
.filter(file => file.endsWith('.js') && file !== 'index.js');
// Files that aren't models or are handled specially
const excludedFiles = ['role-utils.js']; // Utility file, not a model
const specialCases = {
'base.js': 'Base' // base.js is exported as Base, not via spread operator
};
// Check each model file
modelFiles.forEach((file) => {
if (excludedFiles.includes(file)) {
return; // Skip excluded files
}
const modelName = file.replace('.js', '');
if (specialCases[file]) {
// Handle special cases
const exportName = specialCases[file];
should.exist(models[exportName], `Special case ${file} should be exported as models.${exportName}`);
} else {
// Normal model files
// Check if requiring the file produces exports that exist in models
const modelExports = require(`../../../../core/server/models/${modelName}`);
Object.keys(modelExports).forEach((exportKey) => {
// Find this export in the models object
const hasMatch = Object.values(models).some(modelValue =>
modelValue === modelExports[exportKey]);
should(hasMatch).be.true(`Model export ${exportKey} from ${file} should be included in models index`);
});
}
});
});
it('has a deprecated init function', function() {
should.exist(models.init);
models.init.should.be.a.Function();
// In non-production, it should log a warning
const originalConsoleWarn = console.warn;
const originalNodeEnv = process.env.NODE_ENV;
let warningLogged = false;
try {
console.warn = function(message) {
if (message.includes('models.init() is deprecated')) {
warningLogged = true;
}
};
process.env.NODE_ENV = 'development';
models.init();
warningLogged.should.be.true('A deprecation warning should be logged in non-production');
} finally {
console.warn = originalConsoleWarn;
process.env.NODE_ENV = originalNodeEnv;
}
});
});This follows the existing BDD patterns in the codebase and properly handles special cases like role-utils.js, which appears to be a utility file rather than a model. The test verifies that all model exports are properly included in the main models object.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs https://ghost.slack.com/archives/C02G9E68C/p1740997729615089 ref https://linear.app/ghost/issue/ENG-2071/remove-modelsinit-pattern
models.initwill dynamically require all the files in the folder and re-export themmodels.initin unit tests