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

feat: output types & list items are now nullable by default #508

Merged
merged 5 commits into from Sep 30, 2020

Conversation

Weakky
Copy link
Member

@Weakky Weakky commented Sep 22, 2020

This PR

  • makes output types nullable by default
  • makes list items nullable by default

BREAKING CHANGE:
- output types are now nullable by default. Type! -> Type
- list items are now nullable by default. [Type!] -> [Type]
@@ -1147,7 +1147,7 @@ export class SchemaBuilder {
protected decorateList<T extends GraphQLOutputType | GraphQLInputType>(type: T, list: true | boolean[]): T {
let finalType = type
if (!Array.isArray(list)) {
return GraphQLList(GraphQLNonNull(type)) as T
Copy link
Member Author

Choose a reason for hiding this comment

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

bug fix for making list items non-nullable by default

@@ -420,7 +420,7 @@ export class SchemaBuilder {
constructor(protected config: BuilderConfig) {
this.nonNullDefaults = {
input: false,
output: true,
output: false,
Copy link
Member Author

Choose a reason for hiding this comment

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

makes output types nullable by default

@Weakky Weakky changed the title update snapshots feat: output types & list items are now nullable by default Sep 22, 2020
Copy link
Member

@jasonkuhrt jasonkuhrt left a comment

Choose a reason for hiding this comment

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

LGTM

Would be nice if @tgriesser can have a look too

We should merge this week I think.

resolve: () => [1, 2, null],
}),
queryField('userList', {
type: 'User',
list: true,
list: [true],
Copy link
Member

Choose a reason for hiding this comment

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

The array syntax for list has always felt kind of awkward. Was thinking we could deprecate the list property on the object here, instead adding list and nonNull helper fns similar to graphene https://docs.graphene-python.org/en/latest/types/list-and-nonnull/, so this would become:

import { list, queryField } from '@nexus/schema'

queryField('userList', {
  type: list('User'),
  resolve: () => [null, Promise.resolve(null), null],
})

Haven't taken a shot at it yet, but I don't think it'd be too difficult to type / unwrap as necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Could we defer this change to another issue/PR. Agree that we have room to improve here. But I don't think this is necessary to tackle here or in order to ship nullability defaults change.

@tgriesser OK with you?

Copy link
Contributor

Choose a reason for hiding this comment

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

The array syntax for list has always felt kind of awkward. Was thinking we could deprecate the list property on the object here, instead adding list and nonNull helper fns similar to graphene https://docs.graphene-python.org/en/latest/types/list-and-nonnull/, so this would become:

import { list, queryField } from '@nexus/schema'

queryField('userList', {
  type: list('User'),
  resolve: () => [null, Promise.resolve(null), null],
})

Haven't taken a shot at it yet, but I don't think it'd be too difficult to type / unwrap as necessary.

Why to drop it? Is it hard to keep those two options?

Copy link
Member

Choose a reason for hiding this comment

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

@homoky we can keep it around for a bit (or forever) I suppose - just like the idea of consolidating on one way to do things when it's practical.

@Weakky Weakky merged commit c7eff85 into develop Sep 30, 2020
@Weakky Weakky deleted the feat/nullable-defaults branch September 30, 2020 13:53
tgriesser added a commit that referenced this pull request Nov 2, 2020
Fixes #588 by ensuring that list items follow `nonNullDefaults`, which was lost in the #508 refactor

Fixes #384 by removing explicit nullability config from the connection `edges` definition, and also allowing `nonNullDefaults` to be supplied for the connection "types" generated by the creation of a connection field. Allows you to configure both globally in the connection plugin field config, and when the connection field is defined.
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.

None yet

4 participants