Skip to content
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

Stop allowing "unclear types" with Flow #30

Closed
thibaudcolas opened this issue Jan 24, 2019 · 1 comment
Closed

Stop allowing "unclear types" with Flow #30

thibaudcolas opened this issue Jan 24, 2019 · 1 comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@thibaudcolas
Copy link
Owner

thibaudcolas commented Jan 24, 2019

Bug/documentation issue. Unclear types (Array, Object, Function) shouldn't be allowed in the code.

What is the current behavior?

At the moment, .flowconfig turns off the unclear-type linting rule, which allows code to be written with types like Array<Object>. This is problematic because those types end up in the documentation, and aren't really helpful when the API signature of functions actually expects specific shapes for those objects.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. GIFs and screenshots are very helpful too.

For example – shouldKeepEntityByAttribute declares it expects an Array of Objects:

export const shouldKeepEntityByAttribute = (
entityTypes: Array<Object>,
entityType: string,
data: Object,
) => {

the whitelist attribute’s values are actually string | boolean. This is not clear in the type definition of shouldKeepEntityByAttribute, and as a result the type definition of filterEditorState is out of date:

// Refine which entities are kept by whitelisting acceptable values with regular expression patterns.
whitelist: {
[attribute: string]: string,

What is the expected behavior?

All functions should be typed with exact types.

Which versions of the filters or of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of the filters / Draft.js?

This has always been the case. It’s more problematic now because the types are used to generate documentation (#26), which is not as useful as it could be / wrong / imprecise.

@thibaudcolas thibaudcolas added bug Something isn't working help wanted Extra attention is needed good first issue Good for newcomers labels Jan 24, 2019
@thibaudcolas thibaudcolas added this to the v2.3.0 milestone Jan 24, 2019
thibaudcolas added a commit that referenced this issue Jan 25, 2019
- Replaced Object type annotations with the required shape (not exact, can have extra attributes for convenience). This might raise type issues, but only if there is indeed a problem in the library usage. See [Flow’s `unclear-type`](https://flow.org/en/docs/linting/rule-reference/#toc-unclear-type) rule for further reference.
- Use [$ReadOnlyArray<T>](https://flow.org/en/docs/types/arrays/#toc-readonlyarray) in place of Array<T>. This is a supertype of Array (so no change required in implementation), which guarantees draftjs-filters does not mutate arrays.
thibaudcolas added a commit that referenced this issue Jan 25, 2019
- Replaced Object type annotations with the required shape (not exact, can have extra attributes for convenience). This might raise type issues, but only if there is indeed a problem in the library usage. See [Flow’s `unclear-type`](https://flow.org/en/docs/linting/rule-reference/#toc-unclear-type) rule for further reference.
- Use [$ReadOnlyArray<T>](https://flow.org/en/docs/types/arrays/#toc-readonlyarray) in place of Array<T>. This is a supertype of Array (so no change required in implementation), which guarantees draftjs-filters does not mutate arrays.
thibaudcolas added a commit that referenced this issue Jan 25, 2019
## [2.2.2](v2.2.1...v2.2.2) (2019-01-25)

### Bug Fixes

* **api:** use more precise Flow types for public APIs. Fix [#30](#30) ([f274aaa](f274aaa))
@thibaudcolas
Copy link
Owner Author

🎉 This issue is fixed in v2.2.2, available on npm: draftjs-filters@2.2.2.

Generated by 📦🚀 semantic-release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant