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

support defaultValue for input types #203

Merged
merged 18 commits into from Dec 15, 2018

Conversation

Projects
2 participants
@benawad
Copy link
Contributor

commented Nov 22, 2018

fix: #53
improved on: #141
I'm happy to iterate on this further if any changes are needed.

@codecov

This comment has been minimized.

Copy link

commented Nov 22, 2018

Codecov Report

Merging #203 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
+ Coverage   95.33%   95.39%   +0.06%     
==========================================
  Files          59       60       +1     
  Lines         964      978      +14     
  Branches      166      169       +3     
==========================================
+ Hits          919      933      +14     
  Misses         44       44              
  Partials        1        1
Impacted Files Coverage Δ
src/schema/utils.ts 82.35% <ø> (ø) ⬆️
src/schema/schema-generator.ts 97.1% <100%> (+0.14%) ⬆️
src/errors/ConflictingDefaultValuesError.ts 100% <100%> (ø)
src/errors/index.ts 100% <100%> (ø) ⬆️
src/helpers/types.ts 95% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ca81ec...d345838. Read the comment docs.

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

Looks ok but it's just a beginning.
Please take a look at this comment:
#141 (review)

@benawad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

What is Support for initializers in classes ("reflection") - creating an instance of the class and checking the value of the properties to detect default values?

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

@benawad

@InputType()
class SampleInput {
  @Field()
  implicitDefaultField: string = "implicitDefaultFieldValue"

  @Field({ defaultValue: "explicitDefaultFieldValue" })
  explicitDefaultField: string;
}
input SampleInput {
  implicitDefaultField: String = "implicitDefaultFieldValue"
  explicitDefaultField: String = "explicitDefaultFieldValue"
}

@19majkel94 19majkel94 added this to In review in Board via automation Nov 22, 2018

@benawad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

How should I go about doing that?

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Nov 24, 2018

@benawad
Just by "creating an instance of the class and checking the value of the properties to detect default values" 😃

class SampleInput {
  normalField: string;
  stringFieldWithDefault: string = "defaultValue"
}

const instance = new SampleInput();

const defaultsMap = Object.keys(instance).map(fieldName=> ({
  fieldName,
  defaultValue: instance[fieldName],
}));

console.log(defaultsMap);
@benawad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2018

I was able to get it working for input objects, but I'm having trouble figuring out how to do it with args

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Nov 24, 2018

@benawad
You need to do the same things as with input - just create the instance in mapArgFields, read the defaults and attach them to args[field.schemaName] config:

const instance = new (argumentType.target as any)();
@19majkel94

This comment has been minimized.

Copy link
Owner

commented Nov 24, 2018

We should also test some edge cases, like overwriting default values in child classes (implicit and by decorator option):

class Base {
  prop = "baseProp"
}

class Child extends Base {
  prop = "childProp"
}

And I think that we should report to user when implicitDefaultValue has the value and field.typeOptions.defaultValue has the value and they're not equal to each other.
Now it's just silently overwrites the default value from decorator.

@benawad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2018

Is it possible to detect the default values for function parameters?

@Arg("implicitDefaultStringArg")
implicitDefaultStringArg: string = "implicitDefaultStringArgDefaultValue",
@19majkel94

This comment has been minimized.

Copy link
Owner

commented Nov 25, 2018

It is possible by calling .toString() do get the source code and parse it but it might not work when transpiled to ES3/ES5 as it's a runtime operation. Also it's hard to create a good regex that will handle all the combination of function declarations or when the value is a variable, not an explicit string, bool or number. That's why I don't read the arg name using that way, so you have to pass it to the decorator.

@benawad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

Would you like me to make any other changes?

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Nov 27, 2018

@benawad

Would you like me to make any other changes?

Yes, we need to add info about all this things in docs 😃
https://19majkel94.github.io/type-graphql/docs/resolvers.html

  • defaultValue in @Arg decorator
  • defaultValue in @Field decorator (only for @InputType and @ArgsType)
  • initialization of property affects default value in schema
  • inheritance of default values
  • warning about conflict between decorator and and initializer default value

Also, examples and docs might need some updates when there's a default value together with { nullable: true } decorator's option.

And how do we handle the edge case when parent class has property initializer but child try to overwrites it with decorator option? Does it throw error?
And what about when parent has decorator option and child overwrites it with property initalizer?
Do we even test the simple inheritance with no overwrites? 😕

@benawad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

"And how do we handle the edge case when parent class has property initializer but child try to overwrites it with decorator option? Does it throw error?"
"And what about when parent has decorator option and child overwrites it with property initalizer?"

I think we should take the child defaultValue in both cases without throwing an error because they could be inheriting from a class from a library or something and not know (and don't care) how thaat class set the default value.

"Do we even test the simple inheritance with no overwrites? 😕"

Nope, and I just added a test for it and it failed 😂. Where in the code do you handle inheritance?

I'll start working on the docs next.

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Nov 28, 2018

Where in the code do you handle inheritance?

Before #183 it's all over the place 😆
Now I've realized that @Authorized also might not work with inheritance 😞

Args classes inheritance is here:
https://github.com/benawad/type-graphql/blob/d7659fb0d446d3a35ba7943b7f82a60bb939d3bb/src/schema/schema-generator.ts#L453-L460

Input types inheritance is here:
https://github.com/benawad/type-graphql/blob/d7659fb0d446d3a35ba7943b7f82a60bb939d3bb/src/schema/schema-generator.ts#L330-L336

I think we should take the child defaultValue in both cases

Right! Make sure that the tests are covering this cases 😉

Thanks and keep going! 💪

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Dec 8, 2018

@benawad

start working on docs

Are you going to finish this up or should I checkout your branch and add some changes there?

@benawad

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2018

I'm not sure when I'll get time to finish this up, so it would be great if you want to.

19majkel94 added some commits Dec 11, 2018

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Dec 14, 2018

@benawad
I think that we are ready to go with this feature.
Can you take a look at my changes and review them? 😉

@benawad

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2018

I'll take a look at it tonight

@benawad

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2018

Looks good

@19majkel94 19majkel94 merged commit 44e8f89 into 19majkel94:master Dec 15, 2018

3 checks passed

codecov/patch 100% of diff hit (target 95.33%)
Details
codecov/project 95.39% (+0.06%) compared to 7ca81ec
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Board automation moved this from In review to Done Dec 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.