Skip to content
This repository has been archived by the owner on Aug 13, 2020. It is now read-only.

Add flow typing: Part One #20

Merged
merged 27 commits into from
Sep 25, 2019
Merged

Add flow typing: Part One #20

merged 27 commits into from
Sep 25, 2019

Conversation

somewhatabstract
Copy link
Member

This is the first part of adding flow typing to react-render-server.

I decided to submit this PR now as it was getting pretty darn big. In this PR we:

  • Add flow-bin and flow-typed
  • Add linting to ensure files declare @flow or @noflow
  • Add flow-types to arguments.js, logging.js, secret.js and main.js
  • Reworked arguments.js to make mocking argument values easier
  • Reworked secret.js tests to use async/await
  • Setup chai-as-promised for all tests rather than just render_test.js

During this work, I've noticed that flow typing of Winston stuff seems pretty weak. There are also no flow-typed files for some of our other dependencies, including google cloud things. I have filled in some typing gaps for chai-as-promised and winston, but nothing serious; just copy/paste/edit from the typescript files. Another reason for getting this out now is so that subsequent typing edits, which may be more substantial, are clear and easy to review.

Copy link
Member

@jeresig jeresig left a comment

Choose a reason for hiding this comment

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

Nice! This seems like a great start! A couple questions inline, but nothing blocking.

.eslintrc Show resolved Hide resolved
src/logging.js Outdated Show resolved Hide resolved
src/logging.js Outdated Show resolved Hide resolved
src/logging.js Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/secret.js Outdated Show resolved Hide resolved
src/secret.js Show resolved Hide resolved
src/secret_test.js Outdated Show resolved Hide resolved
src/types.js Show resolved Hide resolved
Copy link
Contributor

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

This is awesome. It's great to see Flow being used in more of our projects.

src/fetch_package_test.js Outdated Show resolved Hide resolved
src/arguments.js Show resolved Hide resolved
.flowconfig Show resolved Hide resolved
src/types.js Show resolved Hide resolved
@somewhatabstract somewhatabstract changed the base branch from babeltime to master September 25, 2019 18:25
Also added eslint-plugin-disable to disable react plugin, and added a react plugin version setting so that it stops complaining about react in the output
@somewhatabstract
Copy link
Member Author

Thanks for the feedback, folks. I've addressed all the comments, I believe. I'm going to merge this in and continue with the rest of the changes.

@somewhatabstract somewhatabstract merged commit c534a3e into master Sep 25, 2019
@somewhatabstract somewhatabstract deleted the flowtypes branch September 25, 2019 20:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants