Updated react-error-overlay to latest Flow (0.54.0)

[duvet86]

Update react-error-overlay package to the latest flow, version 0.54.0.
Few unit tests are still failing. See screenshots:
Before 0.54:

After applying 0.54:

Cheers
Luca

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[gaearon]

Why are unit tests failing?

[duvet86]

Hi, the unit tests that are failing are the same that were failing on the master branch.
Regarding the reason why they are failing I am investigating and fixing them now.
Cheers

[Timer]

They seem to be passing in master. Is your tree clean and have you ran npm install?

[duvet86]

My steps are:

• cloning create-react-app
• cd into react-error-overlay
• npm install
• npm test

This is the result:

What am I doing wrong?

[Timer]

You must run npm install in the root of the directory, you should not be running it within react-error-overlay.

[duvet86]

I'm running npm install in the root folder.
Then if I do npm test in the root folder I get:

If I run npm test into react-overlay-error folder I get the previous output.

[Timer]

That's really odd, that's from a fresh clone?
I'm not sure what's going on ... maybe some weird windows quirks due to line endings?

We might need to add a .gitattributes file to force files to be checked out using LF.
Or maybe Jest needs to normalize endings .. (does Jest already normalize endings?)

Can you test if checking out as LF fixes this?
You should be able to do git config --global core.autocrlf false and make a fresh clone, followed by trying again.

Make sure you run git config --global core.autocrlf true after trying this!

[duvet86]

You are absolutely right. Adding core.autocrlf false fixes the issue and all the unit tests pass.
I'll fix and commit the code tonight.
Cheers Luca

[gaearon]

Style nits.

[gaearon]

Can you please leave them typed as React.Element instead of import type? For explicitness (Element is a global in Flow that means DOM element).

[duvet86]

Without the import flow fails:

Flow doc suggests to import React as a namespace with import * as React from 'react' instead of as a default with import React from 'react'.
https://flow.org/en/docs/react/components/
But I thought it was a big change and I decided not to do it.
Cheers

[duvet86]

I could use this syntax: React$Element.

[gaearon]

Oh, I guess you'd have to import * as React from 'react' at which point you'd bump into a false positive warning about accessing PropTypes in 15. Let's keep it this way then.

[gaearon]

Can you do import type { Element as ReactElement } from 'react'?

[gaearon]

Same here (and everywhere below).

[gaearon]

Can you please just add export to type definition itself?

[duvet86]

Apparently I can't.

[duvet86]

Or you probably meant:

export type ErrorRecord = {|
  error: Error,
  unhandledRejection: boolean,
  contextSize: number,
  stackFrames: StackFrame[],
|};

[gaearon]

Yes, that's what I meant.

[gaearon]

Could you please always add newline before import types?

import x from 'x';
import y from 'y';

import type { z } from 'z';

// code

[gaearon]

Same here (and below).

[gaearon]

Why did this change? Seems like it's more permissive than it used to be.

[duvet86]

That was a stupid hack to fix this:

Basically launchEditorEndpoint prop is defined as nullable but flow doesn't understand that the check if (!this.props.launchEditorEndpoint) gets done in canOpenInEditor()

[duvet86]

Sorry I have no idea how to fix this other than adding the same condition twice.

if (!this.props.launchEditorEndpoint && !this.canOpenInEditor()) {
      return;
    }

[gaearon]

You can replace canOpen function with a getEditorEndpoint(): null | string function that returns null if it can't open. Then you can use this function in both places. Since it returns an endpoint you can exit if it's null early and now Flow knows it's a string.

[duvet86]

I think I don't understand, sorry.
This is what I've done but it doesn't work and it still more permissive than before.

[duvet86]

Sorry still not working:

[gaearon]

It's not working because you're not using the return value and instead read the same thing again from props. Flow doesn't know the props are immutable so it's bailing out thinking they might have changed. I am suggesting to use the return value of getEditorEndpoint that you currently ignore. If you compare it against null first and exit if it is, but then use it Flow will know it can't possibly be null.

[duvet86]

Sorry again, I'm even using the double equals to check for undefined and null but:

[gaearon]

You are calling the function twice. Flow can’t know that the value hasn’t changed since last call.

You need to do this:

const endpointUrl = this.getEndpointUrl();
if (endpointUrl === null) {
  return;
}

// ...

// By now Flow knows endpointUrl can't possibly be null
fetch(
  `${endpointUrl}/...`
)

[duvet86]

Thank you, now I understand.

outdated

[gaearon]

Please let's explicitly type this as : string | null.

[gaearon]

Let's add || null here so that it falls back to explicit null for bad values.

[duvet86]

Why making the distinction between null and undefined?

[gaearon]

Then we can use strict === null check here.

[gaearon]

And here.

[duvet86]

Hi, why making the distinction between null and undefined?

[gaearon]

It's easier to know there is just one "bad" value than two. And types can enforce that.

[gaearon]

Have you verified it still works as expected?

[duvet86]

How do I verify that it works as expected?
I create an app with my branch and I break the code to see if error gets displayed as expected?

[gaearon]

You can just build it (npm start in packages/react-error-overlay), then npm start in the root. Then edit packages/react-scripts/template/src/App.js to throw a runtime exception and check that it works.

[duvet86]

[gaearon]

This is great. Thanks!