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

fix: enable non-nullable, defaulted fields in code-first schema #2500

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

sam-super
Copy link
Contributor

@sam-super sam-super commented Nov 9, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

When a not-nullable field has a defaultValue it generates a nullable input field:

@InputType()
export class AcmeInput {
  @Field({ nullable: false, defaultValue: 'foo' })
  bar: string;
}

it generates:

input AcmeInput {
  bar: String = "foo"
}

What is the new behavior?

The above will generate this schema:

input AcmeInput {
  bar: String! = "foo"
}

Does this PR introduce a breaking change?

  • No

Other information

Some of the tests (e.g. the snapshots) capture the erroneous functionality, so to keep this PR specific to the change, some existing fields had nullable: true set to match the existing tests.

@kamilmysliwiec
Copy link
Member

lgtm

@kamilmysliwiec kamilmysliwiec merged commit 2baa066 into nestjs:master Dec 7, 2022
@piotr-pawlowski
Copy link

This "fix" doesn't make sense to me. Now Apollo Kotlin doesn't allow to omit params with default values. Before this fix I could write foo = Input.absent() and I knew that the default value would be applied. Now I have to write foo = "bar", so my app needs to know what is default value for given param.
The second thing is that you wrote: "Does this PR introduce a breaking change? No". It's not true because from now on parameters with default values are non-nullable. So graphql-inspector marks these fields as breaking changes.
To keep backward compability I am required to add explicitly { nullable: true } to each field.

CarsonF added a commit to SeedCompany/cord-api-v3 that referenced this pull request Feb 1, 2023
CarsonF added a commit to SeedCompany/cord-api-v3 that referenced this pull request Feb 1, 2023
GQL schema breaking changes expected but shouldn't cause issues practically

Related to nestjs/graphql#2500
@zq0904
Copy link

zq0904 commented Mar 22, 2023

I was using "@nestjs/graphql": "^10.1.3 ", but the online version updates automatically to 10.2.0

I have a large number of similar inputtypes

For example:
@inputType()
export class ProjectsOptPagingInput extends ProjectOptPagingInput {
@field(() => Int)
projectId: Project['id'] = 0;
}

This change is a destructive change. I would like to know why it was modified. Is the same with version 11?

@wodka
Copy link

wodka commented Mar 27, 2023

How can this be in a patch release? it completely breaks the old behaviour of defaultValue! Now per default all are mandatory, whereas they have been optional!

-> this should be mentioned as a breaking change and should have been released in v11!

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

Successfully merging this pull request may close these issues.

5 participants