facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.07k stars 1.85k forks source link

Inheritance Issues, Generic Typing and the FileReader #3470

Open JoshSmithCode opened 7 years ago

JoshSmithCode commented 7 years ago

Take the following example:

const fileReader: FileReader = new FileReader();
fileReader.addEventListener('load', handleFileLoad);
fileReader.readAsDataURL(file);

const handleFileLoad = (event: ProgressEvent) => {

        const fileReader: FileReader = event.target;
        console.log(fileReader.result);
};

This actually breaks when checked by flow as ProgressEvent.target must be of the type EventTarget.

However, FileReader extends EventTarget, but an error is still thrown when trying to assume this. It is possible to access the event.target as if its a FileReader by wrapping the access as such;

if(event.target instanceof FileReader) { }

But in this implementation, any object could be checked for, there seems to be no correlation between checking for objects that extend the intended EventTarget type.

Is there a reason why it isn't possible to use class inheritance to satisfy the type? Since FileReader extends the EventTarget, is there a way it could be accepted as well as other possible EventTarget extensions?

Additionally, would it instead be better to type-hint ProgressEvents or other Event types less generically to better handle cases like this?

For example if the file reader returned a FileReaderProgressEvent instead of a generic ProgressEvent, it could then expect it's event.target to be a FileReader instead of a generic EventTarget.

asolove commented 7 years ago

This is a pretty frustrating issue. I feel it a lot because even for regular html events like click, the event.target is just an EventTarget and has to be cast to an HTMLElement.

The reason things don't work in this case is that ProgressEvents can have different types of targets. In your use-case, it's a FileReader, but there are also DOM progress events where the target might be a DOM element. So at the type level, just because Flow knows it's a progress event doesn't mean it can assume the target will be any more specific. And there are tons of possible combinations of event names, event targets, and types of thing that the target should maybe be.

So, when you try to type your handler function with a more specific type, saying that it demands to be given an event with a more specific target, Flow (correctly!) says that that doesn't type check, because the code calling the function will possibly provide a target of a different type.

On the browser side, I'm pretty used to just doing the cast to HTMLElement inside the event handler and then not worrying about it. I suspect the right solution is the same here, too.