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.

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

types: implement Schema.discriminator #11855

Merged
merged 4 commits into from Jun 7, 2022

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented May 30, 2022

Resolves #11837

@Uzlopak Uzlopak requested a review from vkarpov15 May 30, 2022 09:36
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented May 30, 2022

@simon-abbott
@mohammad0-0ahmad

lib/schema.js Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented May 30, 2022

Regarding your comment returning void. Actually by using discriminator we mutate the schema. Maybe it would ne better to actually instantiate a new Schema instance and apply the discriminator on it. Idk. Is it possible to type mutation in typescript?

lib/schema.js Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented May 31, 2022

applied changes

@Uzlopak Uzlopak changed the title implement Schema.discriminator types: implement Schema.discriminator May 31, 2022
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.

One quick tweak, otherwise LGTM 👍

Schema.prototype.discriminator = function(name, schema) {
this._applyDiscriminators = {};
this._applyDiscriminators[name] = schema;
this._applyDiscriminators = Object.assign({}, this._applyDiscriminators, { [name]: schema });
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no need to create a new object every single time. Just Object.assign(this._applyDiscriminators, { [name]: schema }); should be enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unit Tests fail if i apply your Suggestion

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't see why. But I'll merge this and try it out for myself.

@mohammad0-0ahmad
Copy link
Contributor

mohammad0-0ahmad commented Jun 5, 2022

I've not used this before actually, but according to the docs or "JS-docs" the example shows that discriminator will return a model, and it is a model FN.
But the ts-tests in this PR calls discriminator by schema instead, and it returns a schema as well.
So I think either the docs should be fixed or these tests or maybe add documentation for schema.discriminator.
Sorry if I've misunderstood the issue here.

Schema.prototype.discriminator = function(name, schema) {
this._applyDiscriminators = {};
this._applyDiscriminators[name] = schema;
this._applyDiscriminators = Object.assign({}, this._applyDiscriminators, { [name]: schema });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't see why. But I'll merge this and try it out for myself.

@vkarpov15 vkarpov15 added this to the 6.3.6 milestone Jun 7, 2022
@vkarpov15 vkarpov15 merged commit f82f7df into Automattic:master Jun 7, 2022
vkarpov15 added a commit that referenced this pull request Jun 7, 2022
@Uzlopak Uzlopak deleted the type-discriminator branch June 7, 2022 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema.discriminator() not defined in TypeScript
4 participants