-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add flow type definitions #91
Conversation
v2.1.0 includes a semver-major change to protocol-buffers-schema that changes how enums are handled (mafintosh/protocol-buffers-schema#24) and breaks the tests.
3ec540c
to
f203abd
Compare
f203abd
to
0f368e3
Compare
@@ -37,13 +37,14 @@ | |||
"homepage": "https://github.com/mapbox/pbf", | |||
"dependencies": { | |||
"ieee754": "^1.1.6", | |||
"resolve-protobuf-schema": "^2.0.0" | |||
"resolve-protobuf-schema": "~2.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for the range change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explained in the commit comment here: 8c01f6b
readBytes(): Uint8Array; | ||
} | ||
|
||
declare module.exports: typeof Pbf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only covers a fraction of the methods, mostly ones for reading, while there are also many for writing. We need to cover all the methods to be able to merge the PR.
|
||
|
||
|
||
Found 1 error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this further, I'm not comfortable with this approach to testing the existence of flow types.
- It will constantly break between flow versions when the output changes in subtle ways.
- It doesn't really test whether the library provides all the necessary flow types — only a few selected ones in
flow.input.js.flow
.
Instead, is there a way to test flow through the actual tests that exercise all the code paths in the library?
Closing -- as @mourner points out, this will be brittle unless we're using Flow more comprehensively in the repo. |
No description provided.