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

[v2] Fixed work with flow >= 0.85, update to 0.91 #3350

Merged
merged 1 commit into from
Jan 30, 2019

Conversation

DragorWW
Copy link
Contributor

@DragorWW DragorWW commented Jan 14, 2019

Fixed work with flow >= 0.85, update to 0.90

Since with flow 0.85, it has become stricter, and requires explicit typing of all export methods and variables.

Changelog to support flow

After the upgrade, flow is displayed 58 new issues, most of them refer to external library and to the HOC manageState.

  • add to flow ignore @atlaskit/layer-manager due to broken types (components/FocusLock/index.js.flow, components/gateway/components/Gateway.js.flow, components/withRenderTarget.js.flow)

Fixed typing in code and also made minor changes in code of docs

Logic problems in code was fixed

New version flow found the following errors in logics:

  • manageState - pass props to Select component incorrectly
  • Select props isMulti:
    • There were problems due to the non-explicit work of the select
  • Creatable: isValidNewOption
  • Select: getOptionLabel and getOptionValue return React.Node

All changes are needed to work with the flow >= 0.85

Other moments

  • SelectAnimated pass incorrect props to Select.componets

P.S.

In my opinion, the required change should not have been so large, a lot of work was done because of old versions of libraries in dependencies

Idea of ​​using types from the npm library (not from flow-typed) is bad, and creates problems

Perhaps it makes sense to divide the pull request into 2 or more, for example, separately babel and separate flow

I am waiting for feedback, maybe I missed something, ready to discuss and correct

@DragorWW
Copy link
Contributor Author

In the near future, I’ll see what the problem is with reduced coverage. Possible problem after updating babel or there is an error in the code.

@DragorWW
Copy link
Contributor Author

DragorWW commented Jan 17, 2019

I checked the problem with reduced coverage.

The reason is that istanbul with babel@6 calculates it now incorrectly

Example: src/components/Menu.js

49 [] |export function getMenuPlacement(...) {
58 [] |    const { spacing } = theme;
       |   // ...
63 [] |    if (!menuEl || !menuEl.offsetParent) return defaultState;
67 [X] |    const { height: scrollHeight } = scrollParent.getBoundingClientRect();
       |    // ...
90 [X] |    switch (placement) {
       |        // ...
152    |        case 'top':
153    |        // 1: the menu will fit, do nothing
154[] |        if (viewSpaceAbove >= menuHeight) {
155[] |            return { placement: 'top', maxHeight };
156    |        }
       |     // ...
       |    }
       |    // ...
       |}

Switch (line 154) cannot be covered because the code after the first return (line 67) is not used

So now coverage is calculated correctly in the version of babel@7

real coverage is 64%

Created issue and PR

@gwyneplaine
Copy link
Collaborator

thanks for this @DragorWW, will have a more detailed look at this shortly. The write up and a cursory glance of the diff lgtm so far.

@DragorWW
Copy link
Contributor Author

Released flow 0.91, today I will update the PR to it

@DragorWW DragorWW changed the title [v2] Fixed work with flow >= 0.85, update to 0.90 [v2] Fixed work with flow >= 0.85, update to 0.91 Jan 18, 2019
class Async extends Component<Props, State> {
export const makeAsyncSelect = <C: {}>(
SelectComponent: AbstractComponent<C>
): AbstractComponent<C & AsyncProps> =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use spread instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spread is a good idea if you have exact flow types

@TrySound
Copy link
Contributor

I would better split build tools upgrade and flow types upgrade. Two PRs will be easier to review.

@DragorWW
Copy link
Contributor Author

@TrySound yes it is a good idea, I will move build tools upgrade in a separate pr

@gwyneplaine
Copy link
Collaborator

@DragorWW, ty for this, can you rebase from master?

Changelog:
[Select]
- change type of formatGroupLabel, getOptionLabel, getOptionValue to () => string
- Async: filterOption default null
- StateManager: stop pass defaultInputValue, defaultMenuIsOpen defaultValue to Select
- add types in src/accessibility
- add types in AsyncCreatable
- use AbstractComponent in HOC
- fix untyped type import errors
- remove $FlowFixMe from src/components/Menu.js
[flow]
- update flow to 0.91
- add lint rule: untyped-type-import as error
- add lint rule unclear-type as warn
- add to ignore @atlaskit/layer-manager files
[flow-typed]
- add react-router-dom@4.x.x types
[Docs]
- remove any type in Footer component
- Header: fix incorrect usage getOptionLabel change toformatOptionLabel
[jest]
- update snapshots
@DragorWW
Copy link
Contributor Author

DragorWW commented Jan 25, 2019

@DragorWW, ty for this, can you rebase from master?

rebased to master, and squash commits to one, and already removed some of the outdated edits and minor changes in the code style that were not related to the main task.

What do you think about adding flow check to CI?

@DragorWW
Copy link
Contributor Author

@TrySound @gwyneplaine I propose to select all the flow type edits associated with changing the code in separate PRs
I think it will be easier and faster.

Today I will prepare the first of them

@TrySound
Copy link
Contributor

sure

@gwyneplaine
Copy link
Collaborator

thanks @DragorWW @TrySound this looks good, I'm merging now 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants