Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(model): add mergePlugins option to discriminator #12613

Merged
merged 4 commits into from
Nov 4, 2022

Conversation

lpizzinidev
Copy link
Contributor

Fix #12604

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

A minor comment. Also we'll have to merge this into 6.8, this is a new option

@@ -34,7 +35,7 @@ module.exports = function discriminator(model, name, schema, tiedValue, applyPlu

if (applyPlugins) {
const applyPluginsToDiscriminators = get(model.base,
'options.applyPluginsToDiscriminators', false) || !mergeHooks;
'options.applyPluginsToDiscriminators', false) || !mergeHooks || !mergePlugins;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a fan of how mergePlugins applies here. Seems a little strange that mergePlugins also effectively disables applyPlugins.

Maybe instead we reuse applyPlugins instead of adding a new option?

used applyPlugins option in discriminator helper instead of creating new option mergePlugins
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Thanks 👍

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to disable applyPlugins for discriminator function
2 participants