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 extracting the type from GraphQLNonNull in getProps #460

Merged
merged 6 commits into from
Sep 3, 2018

Conversation

jgoux
Copy link
Contributor

@jgoux jgoux commented Aug 30, 2018

The internal type stored in GraphQLNonNull was never extracted as the whole field needs to be passed according to extractFromNonNull definition.

The internal type stored in `GraphQLNonNull` was never extracted as the whole `field` needs to be passed according to `extractFromNonNull` definition.
@jgoux jgoux requested a review from radekmie as a code owner August 30, 2018 09:01
@jgoux jgoux changed the title Fix extracting the type from NonNull Fix extracting the type from GraphQLNonNull in getProps Aug 30, 2018
@jgoux
Copy link
Contributor Author

jgoux commented Aug 30, 2018

I think we have to decide either extractFromNonNull returns a field or the underlying type only.

This still not pass :

if (
      fieldType instanceof graphql.GraphQLScalarType &&
      fieldType.name === 'Float'
    ) {
      ready.decimal = true
    }

Because to refer to the internal type, we need to do fieldType.type instanceof graphql.GraphQLScalarType

@radekmie radekmie added the Type: Bug Bug reports and their fixes label Aug 30, 2018
@radekmie
Copy link
Contributor

It should return the underlying type, so GraphQLScalarType and friends. Can we start with adding the tests which should fail in the current implementation? Also, I don't clearly understand, when the current code is not working - could you provide an example?

@jgoux
Copy link
Contributor Author

jgoux commented Aug 30, 2018

I added the failing case by making the decimal a non nullable.

Now we can see that we never pass into :

if (fieldType instanceof graphql.GraphQLScalarType && fieldType.name === 'Float') {
    ready.decimal = true;
}

As you say, extractFromNonNull should return the underlying type so I think the implementation should be :

const extractFromNonNull = type => type && type instanceof graphql.GraphQLNonNull ? type.ofType : type;

@radekmie
Copy link
Contributor

Huh, I'm not sure anymore. I'm not really sure about what the implementation should be, but I think that two things are important here: one, we need to pass fieldType in the same manner, two, cases like decimal should be working correctly.

I'll get to it later today.

@radekmie
Copy link
Contributor

OK, so:

const fieldType = extractFromNonNull(field).type;

Seems to work. Can you check if it works for you too?

@jgoux
Copy link
Contributor Author

jgoux commented Sep 3, 2018

It works! 👍

@codecov
Copy link

codecov bot commented Sep 3, 2018

Codecov Report

Merging #460 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #460   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         157    157           
  Lines        1404   1404           
=====================================
  Hits         1404   1404
Impacted Files Coverage Δ
packages/uniforms/src/GraphQLBridge.js 100% <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 3664d63...9ac44d0. Read the comment docs.

@radekmie radekmie merged commit fd61633 into vazco:master Sep 3, 2018
@jgoux jgoux deleted the patch-1 branch September 4, 2018 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug reports and their fixes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants