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

Flow upgrade #4355

Merged
merged 5 commits into from
Aug 27, 2017
Merged

Flow upgrade #4355

merged 5 commits into from
Aug 27, 2017

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Aug 25, 2017

Summary
Tried to upgrade, and using flow-upgrade, but there's still tons of errors in pretty-format tests. I'm guessing I shouldn't just litter the tests with FlowFixMes?

/cc @pedrottimark

Test plan
yarn flow (currently failing)

@pedrottimark
Copy link
Contributor

@SimenB Although I am happy to make these changes, it might be more efficient for you:

  • For React.createElement(undefined) let’s mock the object to eliminate console warning. I think you will need to rebase because of Return UNDEFINED if type neither string nor function in ReactElement plugin #4360 though. While you are at it, can you please replace Unknown element with undefined element type in the test message?

    const elementSymbol = Symbol.for('react.element'); // ADD preceding the following line:
    const testSymbol = Symbol.for('react.test.json')
    
    test('supports undefined element type', () => {
      expect(formatElement({$$typeof: elementSymbol, props: {}})).toEqual(
        '<UNDEFINED />',
      );
    });
  • null props is reported and labelled as bug react in Flow issues: Error when using React.createElement directly facebook/flow#4658 but no errors for {} empty object, so an alternative work-around to $FlowFuxMe comments (couldn’t resist :) is add const NULL = {}; near the beginning of the file and replace all those null with NULL

@SimenB
Copy link
Member Author

SimenB commented Aug 26, 2017

Thanks for the help @pedrottimark!

I chose to use $FlowFixMe for the null bug, so that it can be easily removed whenever flow fixes it in the future (I love how flow errors on ignores that do nothing)

@codecov-io
Copy link

codecov-io commented Aug 26, 2017

Codecov Report

Merging #4355 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4355   +/-   ##
=======================================
  Coverage   55.85%   55.85%           
=======================================
  Files         189      189           
  Lines        6383     6383           
  Branches        6        6           
=======================================
  Hits         3565     3565           
  Misses       2815     2815           
  Partials        3        3
Impacted Files Coverage Δ
...ackages/pretty-format/src/plugins/react_element.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db4465e...ddefa03. Read the comment docs.

@pedrottimark
Copy link
Contributor

You’re welcome. Thank you for explaining. Y’all have a lot to teach me about fluent JS and Flow.

@cpojer cpojer merged commit 559e70d into jestjs:master Aug 27, 2017
@SimenB SimenB deleted the flow-upgrade branch August 27, 2017 08:26
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants