Skip to content

fix flow errors by annotating null prototypes#1048

Merged
asiandrummer merged 1 commit into
masterfrom
flow-fix
Sep 29, 2017
Merged

fix flow errors by annotating null prototypes#1048
asiandrummer merged 1 commit into
masterfrom
flow-fix

Conversation

@asiandrummer

Copy link
Copy Markdown
Contributor

From flow@0.55.0, flow started doing a more strict null prototype check. Fix them by annotating them with:

__proto__: null

@leebyron

Copy link
Copy Markdown
Contributor

I think this would be easier to understand if you defined a utility type for this in jsutils/

export type ObjMap<T> = { [key: string]: T, __proto__: null };

Then you use ObjMap<SomeType> everywhere instead of { [key: string]: SomeType } which I think is easier to understand.

@asiandrummer

Copy link
Copy Markdown
Contributor Author

Fixes and closes #1045.

@asiandrummer asiandrummer merged commit 9a9de3e into master Sep 29, 2017
@asiandrummer asiandrummer deleted the flow-fix branch September 29, 2017 05:03
@leebyron

Copy link
Copy Markdown
Contributor

Looks great! Though I think you may have missed some manual __proto__: null candidates in there

@cloudkite

Copy link
Copy Markdown

This has a bit of an unfortunate side effect of having to add __proto__: null everywhere that you have strongly typed resolve functions

type Args = {
  id: string,
  __proto__: null,
}

async function resolve(root: *, { id }: Args, ctx: Context): Promise<*> {

any work arounds for this?

@asiandrummer

Copy link
Copy Markdown
Contributor Author

@leebyron oops, I only fixed the ones that gave me flow errors at the moment. Do we need to change all of them?

@cloudkite I'm not sure to be honest - I was inspired to make these fixes as a hotfix to unblock other tools depending on graphql-js. Does anyone know how we could fix this more properly? I'm open to suggestions.

@leebyron

leebyron commented Oct 3, 2017

Copy link
Copy Markdown
Contributor

This isn't great - we should probably specifically make Args a prototyped Object to avoid making people type them in this way.

leebyron added a commit that referenced this pull request Oct 3, 2017
This is a follow up to #1048 which addresses an issue where flow typed resolver functions would need to be aware of the fact that arguments were prototype-less. That's something we maintain for internal usage, but is more burdensome for external use. Instead, this is just more cautious when using these values internally, using hasOwnProperty, but now returns prototypefull objects to user code.

Also follows up with a few additional conversions to ObjMap, such that the only remaining uses of `{[string]: mixed}` are for variables and arguments.
leebyron added a commit that referenced this pull request Oct 3, 2017
This is a follow up to #1048 which addresses an issue where flow typed resolver functions would need to be aware of the fact that arguments were prototype-less. That's something we maintain for internal usage, but is more burdensome for external use. Instead, this is just more cautious when using these values internally, using hasOwnProperty, but now returns prototypefull objects to user code.

Also follows up with a few additional conversions to ObjMap, such that the only remaining uses of `{[string]: mixed}` are for variables and arguments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants