flowtype-loader doesn't catch type errors in downstream modules

[pelotom]

Take this example:

File foo.js

// @flow
export function f() {}

File bar.js:

// @flow
import { f } from './foo'
f()

This type checks. Now if I change foo.js to be:

// @flow
export function f(x: number) {}

webpack recompiles and claims all is well, even though we introduced a type error! Running flow on the command line will catch it:

$ flow
src/components/bar.js:3
  3: f()
     ^^^ function call
  3: f()
     ^^^ undefined (too few arguments, expected default/rest parameters). This type is incompatible with
  2: export function f(x: number) {}
                          ^^^^^^ number. See: src/components/foo.js:2

Now if we just touch bar.js flowtype-loader will notice the problem too, but not before.

It seems that when a file is changed, flowtype-loader restricts its typechecking to just that file, ignoring dependent modules where an error may have been introduced by the change. Is flowtype-loader using some kind of exclusion filter when it runs flow to restrict scope to the changed file? It seems like since the Flow service is already doing all the work to efficiently recompile everything in an incremental fashion, no such filter is necessary or desirable.

[torifat]

You are correct. This loader was created as POC for flow support in create-react-app. And, we filtered out those intentionally at that time. We will try fix it in next week. Thanks for reporting.

[CodyReichert]

Hey @torifat -- any tips on where I can look to not filter out those errors? I'll be happy to send a PR, but I'm not super familiar with the plugin or the webpack API.

[torifat]

@gabriel-laet Can you help here, please?

[torifat]

@CodyReichert Gabriel Laet rewrote the whole logic. He will be able to help you better than me. If he's busy then I will try to help you. Feel free to ping back after a while.

[CodyReichert]

I think I've figured it out, actually. I'll send a PR if I can get confirmation that this is the correct behavior:

The errors get filtered out here:

flowtype-loader/plugin.js

Line 53 in 6cd2569

errors = this._flowStatus.errors.filter(function (error) {

So instead of that, I just removed the filter and assigned errors to the full this._flowStatus.errors. Eg,

FlowtypePlugin.prototype._notifyResourceError = function(resource) {
  if (resource.callback) {
    var errors = [];
    if (this._flowStatus && !this._flowStatus.passed) {
      errors = this._flowStatus.errors
    }
    resource.callback(errors, this._options);
  }
};

Does that sound right -- seems to throw correct errors for me.

[CodyReichert]

@torifat - Not a problem - thank you! I posted what I think the solution is above, I'll see if @gabriel-laet has any feedback on that and I'll open a PR.

Here's a diff on my fork: CodyReichert@4f20b2b

[gabriel-laet]

@CodyReichert @torifat that makes a lot of sense, I've kept the filter because the original implementation had something similar, but looking at it now we should definitely pass all errors. good job! 👍