Don't infer types for plugin provided GQL types #5497
Conversation
| @@ -37,7 +37,8 @@ | |||
| "yargs": "^10.0.3" | |||
| }, | |||
| "engines": { | |||
| "yarn": "^1.2.1" | |||
| "yarn": "^1.2.1", | |||
| "node": ">=6.11.5" | |||
There was a problem hiding this comment.
i can remove this, but it's the minimum engine set by webpack4 so i figured it's the bottom of v2 support
| @@ -34,7 +39,7 @@ const isEmptyObjectOrArray = (obj: any): boolean => { | |||
| const isScalar = val => !_.isObject(val) || val instanceof Date | |||
|
|
|||
| const extractTypes = value => { | |||
| if (_.isArray(value)) { | |||
| if (Array.isArray(value)) { | |||
There was a problem hiding this comment.
flow doesn't understand the lodash checks when choosing a branch on a mixed type
| entries.map(entry => { | ||
| invariant( | ||
| Array.isArray(entry.value), | ||
| `this is validated in the previous call` |
There was a problem hiding this comment.
solely to please flow, this should have already been checked at the call site
|
|
||
| const example = {} | ||
| for (let key of allKeys) { | ||
| if (ignoreFields.includes(key)) continue |
| @@ -400,3 +407,7 @@ export function inferObjectStructureFromNodes({ | |||
|
|
|||
| return inferredFields | |||
| } | |||
|
|
|||
| export function inferObjectStructureFromNodes(options: inferTypeOptions) { | |||
There was a problem hiding this comment.
this is to prevent passing in exampleValue from the outside, it was always intended to be internal but existed as an arg for recursion's sake
| @@ -5,6 +5,18 @@ const typeOf = require(`type-of`) | |||
| const util = require(`util`) | |||
| const { findRootNodeAncestor } = require(`./node-tracking`) | |||
|
|
|||
| export type TypeConflictExample = { | |||
There was a problem hiding this comment.
I hopefully didn't change any logic in this file, i was trying to clean up the flow typing and swapped out plain object hashes for a Map, since it's likely to be faster
|
Sounds/looks great to me! @pieh could you give this a review since you've been touching this code a lot more recently. |
|
I defenitely want to go over this when I get back home.
…On Fri, May 25, 2018, 13:04 Kyle Mathews ***@***.***> wrote:
Sounds/looks great to me! @pieh <https://github.com/pieh> could you give
this a review since you've been touching this code a lot more recently.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5497 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZn7XjedTNslkZCXzBWXIAW-KsNCTNcks5t1-VGgaJpZM4UHQYy>
.
|
|
friendly ping :) |
This is a problem as well in v1? |
|
The optimization could be backported easily enough, but i don't have a sense whether it's worth it. I'm not sure if the duplicate data warning has already been backported? |
It was added in PR against master ( #3905 ) and then merged in v2 with rest of master->v2 merges, so yeah I think we should add this to master. |
This ended up optimizing code with the side benefit of fixing #5494
What this is doing is skipping inference for top level node keys where a plugin has already provided a type manually. Which should also mean that you get no warnings for JSON scalars