React-devtools: Update code to latest Flow version and fix violations

[gaearon]

We are an a very old version of Flow. I’d appreciate if somebody could update the version and then go through all violations, either fixing the code or adding suppressions where reasonable.

[tlakomy]

I'm on it 👍 Never had the chance to use flow in action, this will be interesting learning experience

[gaearon]

Sounds good. It won’t be a lot of fun but I bet you’ll learn something. 😄

[gaearon]

You’ll need to bump the version both in .flowconfig and package.json.

[tlakomy]

... 37 more errors (only 50 out of 87 errors displayed)
To see all errors, re-run Flow with --show-all-errors

What have I gotten myself into 😭

[tlakomy]

I better dig into the documentation first 👍

[gaearon]

One important thing to keep in mind is that an error usually has two sides in Flow. Either something is annotated (or inferred) incorrectly, or used incorrectly. So fixing one fixes the other, but you need to decide which side is wrong. Explicitly annotating things sometimes helps to discover where inference produced something you didn’t expect.

[tlakomy]

I got a case like that right from the start:

if (!this.ids.has(element)) {
      this.ids.set(element, guid());
      this.reactElements.set(this.ids.get(element), element);
    }

reactElements is defined as reactElements: Map<ElementID, OpaqueNodeHandle>; and flow throws an error because it is not aware that this.ids.set(element, guid());will set a value and we will not try to amend the Map with an undefined key

[tlakomy]

Could you tell me if this is the proper approach in this case before I dig in to apply fixes for ~80 errors?

getId(element: OpaqueNodeHandle): ElementID {
    if (typeof element !== 'object' || !element) {
      return element;
    }
    if (!this.ids.has(element)) {
      this.ids.set(element, guid());
      // $FlowIssue flow is not aware that the line above ^ will set the value to this.ids
      this.reactElements.set(this.ids.get(element), element);
    }
      // $FlowIssue same as above, this will be always defined
    return this.ids.get(element);
  }

[gaearon]

Removing indirection helps.

const id = guid();
this.ids.set(element, id);
this.reactElements.set(id, element);

[gaearon]

In case of return, you could add an explicit check, e.g.

const id = this.ids.get(element);
if (typeof id !== 'string') {
  throw new Error('Expected ID to exist.');
}
return id;

This way you can “prove” to Flow that this is safe.

[tlakomy]

But what if the element key already has an id? I was under the impression that the if statement is for ensuring that this will not be overwritten.

BTW: How do I enable syntax colors on github? 😄

[tlakomy]

Fair enough, explicit checks sounds good - that way both flow and maintainer will be aware that something went wrong.

[gaearon]

Write ```js for colors.

[tlakomy]

Allright, wish me luck - I'll let you know when I'll have the PR ready. Thanks for all the help, much appreciated 👍

[gaearon]

But what if the element key already has an id? I was under the impression that the if statement is for ensuring that this will not be overwritten.

This should work:

getId(element: OpaqueNodeHandle): ElementID {
    if (typeof element !== 'object' || !element) {
      return element;
    }
    if (!this.ids.has(element)) {
      const id = guid();
      this.ids.set(element, id);
      this.reactElements.set(id, element);
      return id;
    }
    return this.ids.get(element);
  }

if you ensure your map is annotated.

[gaearon]

Oh okay I see what you mean.
The method with catching an error should work then.

[gaearon]

getId(element: OpaqueNodeHandle): ElementID {
    if (typeof element !== 'object' || !element) {
      return element;
    }
    if (!this.ids.has(element)) {
      const id = guid();
      this.ids.set(element, id);
      this.reactElements.set(id, element);
      return id;
    }
    const id = this.ids.get(element);
    if (typeof id !== 'string') { // or whatever
      throw new Error('Expected ID to exist by now.');
    }
    return id;
  }

[tlakomy]

FYI: I'm down to 36 errors but I'd need a bit of guidance regarding some event-related errors. I'll explain that in a PR which will come shortly (hopefully I'll get the time)

[tlakomy]

I'm down to some quite cryptic error messages in BananaSlugBackendManager.js and some other so I decided to open a WIP PR so it can be reviewed in the meantime.

[gaearon]

Re-adding the label since @tomaszlakomygp is not working on this.

[Rudeg]

hey @gaearon, I would be happy to work on it :)

[tlakomy]

@Rudeg Make sure to check out excellent comments made by @suchipi in my PR linked to this issue, unfortunately I won't be able to finish the work I started in a while due to some personal matters. Hopefully it'll be helpful :)

[mannanali413]

@gaearon I can take this up

[gaearon]

@Rudeg You got here first so it’s yours 👍

Feel free to team up with @mannanali413 if you need any help!

[mannanali413]

@Rudeg I will be happy to work along with you on this.

[aarondancer]

Working on this rn as well. I've knocked out about half the violations so far (out of 105).

[gaearon]

@aarondancer Can you please work it out with @mannanali413? He already claimed the issue so it’s not very nice to try to get there first. Ideally if you want to help, you need to coordinate with the person who claimed the issue, and maybe work on it together.

[aarondancer]

Sure thing @gaearon

[gaearon]

@aarondancer

It seems like @mannanali413 and @Rudeg are busy.
Would you like to take over this, as you already started some work on it?

[aarondancer]

Sure. I'll continue where I left off @gaearon

[riseremi]

@aarondancer
Any progress on the issue? Can I pick this up?

[aarondancer]

Go ahead @riseremi. Feel free to take what you want from my fork. I was able to get the errors down to only 9 remaining at the time.

[riseremi]

@aarondancer thank you. 👍 I'll look at your fork tomorrow.

[riseremi]

@gaearon should I extract the flow types to types.js or this file is supposed to contain only generic types? If types.js is for generic types only, where should I put these types?

For example, I want to extract the Pane type from RelayPlugin to use it in some other places:

(node: Object, id: string) => React.Element<*>

I think that just importing the type from RelayPlugin is not a good idea, since there might be other plugins in the future that want to use this type.

[bvaughn]

Frontend and backend types.js files are meant for types that are shared among many files. If you think the Pane type may be used by multiple components, it would be reasonable to relocate that type.

[riseremi]

@bvaughn thank you, Brian.

I've submitted a PR (https://github.com/facebook/react-devtools/pull/884). Please take a look when you'll have time.

There are a lot of changes and workarounds just because I don't know how to do that properly. Also there are merge conflicts and I decided not to solve them by myself, since I may break something. I'll be happy to fix any issues and inaccuracies with the PR.

[bvaughn]

React DevTools has been rewritten and recently launched a new version 4 UI. The source code for this rewrite was done in a separate repository and now lives in the main React repo (github.com/facebook/react).

Because version 4 was a total rewrite, and all issues in this repository are related to the old version 3 of the extension, I am closing all issues in this repository. If you can still reproduce this issue, or believe this feature request is still relevant, please open a new issue in the React repo: https://github.com/facebook/react/issues/new?labels=Component:%20Developer%20Tools