chore: Fix on flow type signature to pass type checking on flow >= v0.132.0 #2054
Conversation
package.json
Outdated
| "src/**", | ||
| "index.mjs" | ||
| "index.mjs", | ||
| "flow-typed/*.js" |
There was a problem hiding this comment.
agreed with @Rob--W to don't export the custom flow definitions for the third party dependencies used internally for now, we can include them if any other package depends from web-ext and would need them.
src/extension-runners/chromium.js
Outdated
| ); | ||
|
|
||
| if (!this.wss) { | ||
| throw new Error('WebSocketServer closed'); |
There was a problem hiding this comment.
What's this doing here? There will be a runtime error at this.wss.address() below, anyway.
Removing this would reduce the need for extra test coverage. (but since you have test coverage anyway, perhaps tweak the error message if you care about the scenario)
There was a problem hiding this comment.
I opted to replace this with a $FlowIgnore suppress comment.
| export class FirefoxAndroidExtensionRunner { | ||
| // Wait 3s before the next unix socket discovery loop. | ||
| static unixSocketDiscoveryRetryInterval = 3 * 1000; | ||
| static unixSocketDiscoveryRetryInterval: number = 3 * 1000; |
There was a problem hiding this comment.
Is Flow really that dumb to not be able to infer the type from this declaration?
There was a problem hiding this comment.
as discussed in the triage meeting: I didn't dig too much, but I got the feeling that flow did enforce explicitly type annotations (instead of just trust what type did flow infer) on what it is being exported explicitly from a module (which to me does make kind of sense: "infer the type used internally as much as possible, enforce explicit type annotation of what is exported outside of a module").
| }); | ||
|
|
||
| const connectClient = async () => { | ||
| if (!runnerInstance.wss) { |
There was a problem hiding this comment.
Similarly there is going to be a runtime error anyway at runnerInstance.wss.address(). Does this help with anything?
There was a problem hiding this comment.
I opted to replace this with a $FlowIgnore suppress comment.
rpl
force-pushed
the
fix/flow-type-signature-flow-ge-0.132.0
branch
from
3d7e21b to
ef7271a
Compare
3 participants
This PR contains a number of changes needed to pass the flowtype checks on flow >= v0.132.0 (see #2003).
Most of the changes are only applied to the flow type signatures, but for some of the flowtype errors I opted to make some small changes on the implementation side (when it did look more appropriate).
Many of the new flowtype checks errors in #2003 are related to the dependencies, it looks that more recent flow versions are detecting when the types used are coming from a dependency that isn't typed using flow and the resulting types are all going to degrate to
any.Unfortunately the npm modules that are triggering these flowtype errors do not have their own flowtype declarations, and there isn't a third party one in the flow-types repository neither.
The easiest approach seems to be to include our own flow-typed declarations files in the project repository.
These declaration files are only including the subset of the modules that is actually being used in the web-ext sources, everything else is currently omitted.