From c3c00238e88347fad6de7cf19982efd017255fdd Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 09:47:28 +0000 Subject: [PATCH 01/71] initial Flow setup --- .flowconfig | 18 ++++++++++++++++++ package.json | 2 ++ src/amo/components/RatingManager/index.js | 1 + yarn.lock | 4 ++++ 4 files changed, 25 insertions(+) create mode 100644 .flowconfig diff --git a/.flowconfig b/.flowconfig new file mode 100644 index 00000000000..c6bb099b3e1 --- /dev/null +++ b/.flowconfig @@ -0,0 +1,18 @@ +[ignore] +/dist/.* + +# These modules opt into Flow but we don't need to check them. +.*/node_modules/babel.* +.*/node_modules/react-nested-status +.*/node_modules/stylelint + +[include] + +[libs] + +[options] +module.system=node +module.system.node.resolve_dirname=./src +log.file=./artifacts/flow.log +suppress_comment= \\(.\\|\n\\)*\\$FLOW_FIXME +suppress_comment= \\(.\\|\n\\)*\\$FLOW_IGNORE diff --git a/package.json b/package.json index 731a04fa43c..a7c66e87b96 100644 --- a/package.json +++ b/package.json @@ -14,6 +14,7 @@ "dev:amo:no-proxy": "better-npm-run dev:amo:no-proxy", "dev:disco": "better-npm-run dev:disco", "eslint": "eslint .", + "flow:check": "flow check", "stylelint": "stylelint --syntax scss **/*.scss", "lint": "npm run eslint && npm run stylelint", "servertest": "bin/config-check.js && ADDONS_FRONTEND_BUILD_ALL=1 npm run build && better-npm-run servertest && better-npm-run servertest:amo && better-npm-run servertest:disco && better-npm-run servertest:admin", @@ -249,6 +250,7 @@ "eslint-plugin-react": "6.10.3", "fetch-mock": "5.9.4", "file-loader": "0.10.1", + "flow-bin": "0.38.0", "glob": "7.1.1", "http-proxy": "1.16.2", "json-loader": "0.5.4", diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index f4cd67dd89f..d2b9178f632 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -1,3 +1,4 @@ +/* @flow */ import React, { PropTypes } from 'react'; import { connect } from 'react-redux'; import { compose } from 'redux'; diff --git a/yarn.lock b/yarn.lock index bc55c58a2b5..2a8d3e06dca 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2595,6 +2595,10 @@ flatten@^1.0.2: version "1.0.2" resolved "https://registry.yarnpkg.com/flatten/-/flatten-1.0.2.tgz#dae46a9d78fbe25292258cc1e780a41d95c03782" +flow-bin@0.38.0: + version "0.38.0" + resolved "https://registry.yarnpkg.com/flow-bin/-/flow-bin-0.38.0.tgz#3ae096d401c969cc8b5798253fb82381e2d0237a" + for-in@^0.1.3: version "0.1.8" resolved "https://registry.yarnpkg.com/for-in/-/for-in-0.1.8.tgz#d8773908e31256109952b1fdb9b3fa867d2775e1" From 36b18a833a46b8ed0a19d5de3fdd8cd36f04428f Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 15:24:31 +0000 Subject: [PATCH 02/71] ignore flow log --- .gitignore | 1 + artifacts/README.md | 1 + 2 files changed, 2 insertions(+) create mode 100644 artifacts/README.md diff --git a/.gitignore b/.gitignore index bf543af8b94..f7ea5b48e5f 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ logs *.log npm-debug.log* +artifacts/flow.log # Runtime data pids diff --git a/artifacts/README.md b/artifacts/README.md new file mode 100644 index 00000000000..2b0352ebc6d --- /dev/null +++ b/artifacts/README.md @@ -0,0 +1 @@ +Ephemeral artifacts are saved to this directory. From e3869f36a2cd2dedc27188f6b9295e4770cdf835 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 17:39:13 +0000 Subject: [PATCH 03/71] more fixups --- .flowconfig | 3 + .gitignore | 2 +- flow/flowStub.js.flow | 1 + flow/logs/README.md | 1 + src/amo/actions/reviews.js | 17 +++++- src/amo/api/index.js | 13 +++- src/amo/components/RatingManager/index.js | 72 +++++++++++++++-------- src/core/types/addonTypes.js | 51 ++++++++++++++++ src/core/types/reduxTypes.js | 3 + 9 files changed, 137 insertions(+), 26 deletions(-) create mode 100644 flow/flowStub.js.flow create mode 100644 flow/logs/README.md create mode 100644 src/core/types/addonTypes.js create mode 100644 src/core/types/reduxTypes.js diff --git a/.flowconfig b/.flowconfig index c6bb099b3e1..4da7fbcec27 100644 --- a/.flowconfig +++ b/.flowconfig @@ -1,4 +1,5 @@ [ignore] +# Ignore minified addons-frontend code. /dist/.* # These modules opt into Flow but we don't need to check them. @@ -11,6 +12,8 @@ [libs] [options] +# This maps all sass imports to a dummy flow file to suppress import errors. +module.name_mapper.extension='scss' -> '/flow/flowStub.js.flow' module.system=node module.system.node.resolve_dirname=./src log.file=./artifacts/flow.log diff --git a/.gitignore b/.gitignore index f7ea5b48e5f..7245f4fac0c 100644 --- a/.gitignore +++ b/.gitignore @@ -2,7 +2,7 @@ logs *.log npm-debug.log* -artifacts/flow.log +flow/logs/*log* # Runtime data pids diff --git a/flow/flowStub.js.flow b/flow/flowStub.js.flow new file mode 100644 index 00000000000..d4fccb74635 --- /dev/null +++ b/flow/flowStub.js.flow @@ -0,0 +1 @@ +// This file is just a stub that will suppress 'missing file' errors. diff --git a/flow/logs/README.md b/flow/logs/README.md new file mode 100644 index 00000000000..4c3fcb28bc1 --- /dev/null +++ b/flow/logs/README.md @@ -0,0 +1 @@ +Flow server logs are written to this directory. diff --git a/src/amo/actions/reviews.js b/src/amo/actions/reviews.js index d8324acb4f4..0b7d0f1c1bb 100644 --- a/src/amo/actions/reviews.js +++ b/src/amo/actions/reviews.js @@ -1,6 +1,21 @@ import { SET_ADDON_REVIEWS, SET_REVIEW } from 'amo/constants'; -export function denormalizeReview(review) { +export type UserReviewType = {| + addonId: number, + addonSlug: string, + body: string, + created: Date, + title: string, + id: number, + isLatest: boolean, + rating: number, + userId: number, + userName: string, + userUrl: string, + versionId: number, +|}; + +export function denormalizeReview(review: Object): UserReviewType { return { addonId: review.addon.id, addonSlug: review.addon.slug, diff --git a/src/amo/api/index.js b/src/amo/api/index.js index ad821fca7ad..5fa024702af 100644 --- a/src/amo/api/index.js +++ b/src/amo/api/index.js @@ -1,6 +1,17 @@ +/* @flow */ import { callApi } from 'core/api'; import log from 'core/logger'; +export type SubmitReviewParams = {| + addonId?: number, + rating?: number, + apiState?: Object, + title?: string, + versionId?: number, + body?: string, + reviewId?: number, +|}; + /* * POST/PATCH an add-on review using the API. */ @@ -13,7 +24,7 @@ export function submitReview({ body, reviewId, ...apiCallParams -}) { +}: SubmitReviewParams) { return new Promise( (resolve) => { const data = { rating, version: versionId, body, title }; diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index d2b9178f632..ee2430aad1e 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -3,10 +3,15 @@ import React, { PropTypes } from 'react'; import { connect } from 'react-redux'; import { compose } from 'redux'; -import { withErrorHandling } from 'core/errorHandler'; +import { ErrorHandler, withErrorHandling } from 'core/errorHandler'; import { setReview } from 'amo/actions/reviews'; +import type { UserReviewType } from 'amo/actions/reviews'; import { getLatestUserReview, submitReview } from 'amo/api'; +import type { SubmitReviewParams } from 'amo/api'; import DefaultAddonReview from 'amo/components/AddonReview'; +import type { DispatchFn } from 'core/types/reduxTypes'; +import type { AddonType, AddonTypeProp, AddonVersionType } + from 'core/types/addonTypes'; import DefaultAuthenticateButton from 'core/components/AuthenticateButton'; import { ADDON_TYPE_DICT, @@ -22,23 +27,34 @@ import DefaultRating from 'ui/components/Rating'; import './styles.scss'; +type GetLogInPromptParams = {| addonType: AddonTypeProp |}; + +type GetLogInPromptOptions = {| + validAddonTypes: typeof defaultValidAddonTypes, +|}; + +type RatingManagerProps = {| + AddonReview: typeof DefaultAddonReview, + AuthenticateButton: typeof DefaultAuthenticateButton, + Rating: typeof DefaultRating, + addon: AddonType, + errorHandler: typeof ErrorHandler, + apiState: Object, + i18n: Object, + loadSavedReview: Function, + location: Object, // might be builtin + submitReview: Function, + userId: number, + userReview: UserReviewType, + version: AddonVersionType, +|}; export class RatingManagerBase extends React.Component { - static propTypes = { - AddonReview: PropTypes.node, - AuthenticateButton: PropTypes.node, - addon: PropTypes.object.isRequired, - errorHandler: PropTypes.func.isRequired, - apiState: PropTypes.object, - i18n: PropTypes.object.isRequired, - loadSavedReview: PropTypes.func.isRequired, - location: PropTypes.object.isRequired, - Rating: PropTypes.node, - submitReview: PropTypes.func.isRequired, - userId: PropTypes.number, - userReview: PropTypes.object, - version: PropTypes.object.isRequired, - } + props: RatingManagerProps; + ratingLegend: Node; + state: {| + showTextEntry: boolean, + |}; static defaultProps = { AddonReview: DefaultAddonReview, @@ -46,7 +62,7 @@ export class RatingManagerBase extends React.Component { Rating: DefaultRating, } - constructor(props) { + constructor(props: RatingManagerProps) { super(props); const { loadSavedReview, userId, addon } = props; this.state = { showTextEntry: false }; @@ -56,7 +72,7 @@ export class RatingManagerBase extends React.Component { } } - onSelectRating = (rating) => { + onSelectRating = (rating: number) => { const { userId, userReview, version } = this.props; const params = { @@ -64,6 +80,7 @@ export class RatingManagerBase extends React.Component { rating, apiState: this.props.apiState, addonId: this.props.addon.id, + reviewId: undefined, versionId: version.id, userId, }; @@ -90,7 +107,8 @@ export class RatingManagerBase extends React.Component { } getLogInPrompt( - { addonType }, { validAddonTypes = defaultValidAddonTypes } = {} + { addonType }: GetLogInPromptParams, + { validAddonTypes = defaultValidAddonTypes }: GetLogInPromptOptions = {} ) { const { i18n } = this.props; switch (addonType) { @@ -166,12 +184,15 @@ export class RatingManagerBase extends React.Component { } } -export const mapStateToProps = (state, ownProps) => { +export const mapStateToProps = ( + state: Object, ownProps: RatingManagerProps +) => { const userId = state.auth && state.auth.userId; let userReview; // Look for the latest saved review by this user for this add-on. if (userId && state.reviews && ownProps.addon) { + // $FLOW_FIXME: support babel magic dedent log.info(dedent`Checking state for review by user ${userId}, addonId ${ownProps.addon.id}, versionId ${ownProps.version.id}`); @@ -194,9 +215,14 @@ export const mapStateToProps = (state, ownProps) => { }; }; -export const mapDispatchToProps = (dispatch) => ({ +type LoadSavedReviewParams = {| + userId: number, + addonId: number, +|}; + +export const mapDispatchToProps = (dispatch: DispatchFn) => ({ - loadSavedReview({ userId, addonId }) { + loadSavedReview({ userId, addonId }: LoadSavedReviewParams) { return getLatestUserReview({ user: userId, addon: addonId }) .then((review) => { if (review) { @@ -208,7 +234,7 @@ export const mapDispatchToProps = (dispatch) => ({ }); }, - submitReview(params) { + submitReview(params: SubmitReviewParams) { return submitReview(params).then((review) => dispatch(setReview(review))); }, }); diff --git a/src/core/types/addonTypes.js b/src/core/types/addonTypes.js new file mode 100644 index 00000000000..c767796064c --- /dev/null +++ b/src/core/types/addonTypes.js @@ -0,0 +1,51 @@ +/* @flow */ +import { + ADDON_TYPE_DICT, + ADDON_TYPE_EXTENSION, + ADDON_TYPE_LANG, + ADDON_TYPE_SEARCH, + ADDON_TYPE_THEME, + validAddonTypes as defaultValidAddonTypes, +} from 'core/constants'; + +// TODO: Are all these fields actually in addon.current_version +export type AddonVersionType = {| + id: number, + license: { name: string, url: string }, + version: string, + files: Array, +|}; + +export type AddonAuthorType = {| + name: string, + url: string, +|}; + +export type AddonTypeProp = + typeof ADDON_TYPE_DICT | + typeof ADDON_TYPE_LANG | + typeof ADDON_TYPE_SEARCH | + typeof ADDON_TYPE_THEME | + typeof ADDON_TYPE_EXTENSION; + +export type AddonType = {| + id: number, + guid: string, + name: string, + icon_url: string, + slug: string, + average_daily_users: number, + authors: Array, + current_version: AddonVersionType, + previews: Array, + ratings: { + count: number, + average: number, + }, + summary: string, + description: string, + has_privacy_policy: boolean, + homepage: string, + support_url: string, + type: AddonTypeProp, +|}; diff --git a/src/core/types/reduxTypes.js b/src/core/types/reduxTypes.js new file mode 100644 index 00000000000..eebe48dde88 --- /dev/null +++ b/src/core/types/reduxTypes.js @@ -0,0 +1,3 @@ +/* @flow */ + +export type DispatchFn = (action: Object) => void; From a6d999f5cd7dfa7a24a0ae28a0e9b2d888ac60f3 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 17:41:43 +0000 Subject: [PATCH 04/71] moved logs --- .flowconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.flowconfig b/.flowconfig index 4da7fbcec27..c851ee1c5cb 100644 --- a/.flowconfig +++ b/.flowconfig @@ -16,6 +16,6 @@ module.name_mapper.extension='scss' -> '/flow/flowStub.js.flow' module.system=node module.system.node.resolve_dirname=./src -log.file=./artifacts/flow.log +log.file=./flow/logs/flow.log suppress_comment= \\(.\\|\n\\)*\\$FLOW_FIXME suppress_comment= \\(.\\|\n\\)*\\$FLOW_IGNORE From bcabe90b8d117ce875544291eeb6445bfb3d735e Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 18:04:37 +0000 Subject: [PATCH 05/71] fixed dedent --- .flowconfig | 2 ++ flow/libs/README.md | 3 +++ flow/libs/dedent.js.flow | 1 + src/amo/components/RatingManager/index.js | 1 - 4 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 flow/libs/README.md create mode 100644 flow/libs/dedent.js.flow diff --git a/.flowconfig b/.flowconfig index c851ee1c5cb..d3b368ac22a 100644 --- a/.flowconfig +++ b/.flowconfig @@ -10,11 +10,13 @@ [include] [libs] +./flow/libs/dedent.js.flow [options] # This maps all sass imports to a dummy flow file to suppress import errors. module.name_mapper.extension='scss' -> '/flow/flowStub.js.flow' module.system=node +module.system.node.resolve_dirname=node_modules module.system.node.resolve_dirname=./src log.file=./flow/logs/flow.log suppress_comment= \\(.\\|\n\\)*\\$FLOW_FIXME diff --git a/flow/libs/README.md b/flow/libs/README.md new file mode 100644 index 00000000000..1cbdc69a8b0 --- /dev/null +++ b/flow/libs/README.md @@ -0,0 +1,3 @@ +The files in this directory (when declared in `[libs]` of `.flowconfig`) +will declare global objects. These declarations should be a last resort. +Try to export/import types in the actual source code first. diff --git a/flow/libs/dedent.js.flow b/flow/libs/dedent.js.flow new file mode 100644 index 00000000000..0c0ad9b6d11 --- /dev/null +++ b/flow/libs/dedent.js.flow @@ -0,0 +1 @@ +declare function dedent(templateString: Array): string; diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index ee2430aad1e..5d11348a215 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -192,7 +192,6 @@ export const mapStateToProps = ( // Look for the latest saved review by this user for this add-on. if (userId && state.reviews && ownProps.addon) { - // $FLOW_FIXME: support babel magic dedent log.info(dedent`Checking state for review by user ${userId}, addonId ${ownProps.addon.id}, versionId ${ownProps.version.id}`); From 776f86c926216b073fcfd00b1883f95d9bc33852 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 18:18:56 +0000 Subject: [PATCH 06/71] api fixes, dedent fix --- flow/libs/dedent.js.flow | 2 +- src/amo/api/index.js | 26 +++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/flow/libs/dedent.js.flow b/flow/libs/dedent.js.flow index 0c0ad9b6d11..744c9b4cc94 100644 --- a/flow/libs/dedent.js.flow +++ b/flow/libs/dedent.js.flow @@ -1 +1 @@ -declare function dedent(templateString: Array): string; +declare function dedent(params: Array<*>): string; diff --git a/src/amo/api/index.js b/src/amo/api/index.js index 5fa024702af..5e49863682c 100644 --- a/src/amo/api/index.js +++ b/src/amo/api/index.js @@ -27,7 +27,13 @@ export function submitReview({ }: SubmitReviewParams) { return new Promise( (resolve) => { - const data = { rating, version: versionId, body, title }; + const data = { + addon: undefined, + rating, + version: versionId, + body, + title + }; let method = 'POST'; let endpoint = 'reviews/review'; @@ -54,7 +60,14 @@ export function submitReview({ }); } -export function getReviews({ user, addon, ...params } = {}) { +type GetReviewsParams = {| + addon: number, + user: number, +|}; + +export function getReviews( + { user, addon, ...params }: GetReviewsParams = {} +) { return new Promise((resolve) => { if (!user && !addon) { throw new Error('Either user or addon must be specified'); @@ -73,7 +86,14 @@ export function getReviews({ user, addon, ...params } = {}) { }); } -export function getLatestUserReview({ user, addon } = {}) { +type GetLatestReviewParams = {| + user: number, + addon: number, +|}; + +export function getLatestUserReview( + { user, addon }: GetLatestReviewParams = {} +) { return new Promise((resolve) => { if (!user || !addon) { throw new Error('Both user and addon must be specified'); From a011fcf4e3961e66ea8f858cdab2d510e16c64b1 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 18:42:02 +0000 Subject: [PATCH 07/71] removed userId, fixed action callbacks --- src/amo/api/index.js | 6 +++++- src/amo/components/RatingManager/index.js | 7 +++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/amo/api/index.js b/src/amo/api/index.js index 5e49863682c..6ea1c3ae9a1 100644 --- a/src/amo/api/index.js +++ b/src/amo/api/index.js @@ -1,11 +1,15 @@ /* @flow */ import { callApi } from 'core/api'; +import { ErrorHandler } from 'core/errorHandler'; import log from 'core/logger'; +// TODO: make a separate function for posting/patching so that we +// can type check each one independently. export type SubmitReviewParams = {| addonId?: number, rating?: number, apiState?: Object, + errorHandler?: typeof ErrorHandler, title?: string, versionId?: number, body?: string, @@ -24,7 +28,7 @@ export function submitReview({ body, reviewId, ...apiCallParams -}: SubmitReviewParams) { +}: SubmitReviewParams): Promise { return new Promise( (resolve) => { const data = { diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 5d11348a215..13cc533456a 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -41,9 +41,9 @@ type RatingManagerProps = {| errorHandler: typeof ErrorHandler, apiState: Object, i18n: Object, - loadSavedReview: Function, + loadSavedReview: (LoadSavedReviewParams) => void, location: Object, // might be builtin - submitReview: Function, + submitReview: (SubmitReviewParams) => Promise, userId: number, userReview: UserReviewType, version: AddonVersionType, @@ -73,7 +73,7 @@ export class RatingManagerBase extends React.Component { } onSelectRating = (rating: number) => { - const { userId, userReview, version } = this.props; + const { userReview, version } = this.props; const params = { errorHandler: this.props.errorHandler, @@ -82,7 +82,6 @@ export class RatingManagerBase extends React.Component { addonId: this.props.addon.id, reviewId: undefined, versionId: version.id, - userId, }; if (userReview) { From b6664e1d74bb6a949491beb02540dd5c75846d1e Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 19:17:28 +0000 Subject: [PATCH 08/71] better function defs --- src/amo/components/RatingManager/index.js | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 13cc533456a..2895923a13a 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -41,9 +41,9 @@ type RatingManagerProps = {| errorHandler: typeof ErrorHandler, apiState: Object, i18n: Object, - loadSavedReview: (LoadSavedReviewParams) => void, + loadSavedReview: LoadSavedReviewFn, location: Object, // might be builtin - submitReview: (SubmitReviewParams) => Promise, + submitReview: SubmitReviewFn, userId: number, userReview: UserReviewType, version: AddonVersionType, @@ -213,14 +213,23 @@ export const mapStateToProps = ( }; }; -type LoadSavedReviewParams = {| +type LoadSavedReviewFn = ({| userId: number, addonId: number, -|}; +|}) => Promise<*>; + +type SubmitReviewFn = (SubmitReviewParams) => Promise; + +type DispatchMappedProps = {| + loadSavedReview: LoadSavedReviewFn, + submitReview: SubmitReviewFn, +|} -export const mapDispatchToProps = (dispatch: DispatchFn) => ({ +export const mapDispatchToProps = ( + dispatch: DispatchFn +): DispatchMappedProps => ({ - loadSavedReview({ userId, addonId }: LoadSavedReviewParams) { + loadSavedReview({ userId, addonId }) { return getLatestUserReview({ user: userId, addon: addonId }) .then((review) => { if (review) { @@ -232,7 +241,7 @@ export const mapDispatchToProps = (dispatch: DispatchFn) => ({ }); }, - submitReview(params: SubmitReviewParams) { + submitReview(params) { return submitReview(params).then((review) => dispatch(setReview(review))); }, }); From 8ea3b1165206d0c74bf78f4805665c09f1904cc3 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 22:00:52 +0000 Subject: [PATCH 09/71] api actions, reducers, review actions --- src/amo/actions/reviews.js | 35 +++++- src/amo/api/index.js | 23 +++- src/amo/components/RatingManager/index.js | 5 +- src/core/actions/index.js | 49 ++++++-- src/core/api/index.js | 113 +++++++++++++++--- .../components/AuthenticateButton/index.js | 54 ++++++--- src/core/reducers/api.js | 47 +++++++- src/core/types/coreTypes.js | 5 + 8 files changed, 279 insertions(+), 52 deletions(-) create mode 100644 src/core/types/coreTypes.js diff --git a/src/amo/actions/reviews.js b/src/amo/actions/reviews.js index 0b7d0f1c1bb..5f988b866e4 100644 --- a/src/amo/actions/reviews.js +++ b/src/amo/actions/reviews.js @@ -1,4 +1,6 @@ +/* @flow */ import { SET_ADDON_REVIEWS, SET_REVIEW } from 'amo/constants'; +import type { ApiReviewType } from 'amo/api'; export type UserReviewType = {| addonId: number, @@ -33,26 +35,51 @@ export function denormalizeReview(review: Object): UserReviewType { }; } -const setReviewAction = (review) => ({ type: SET_REVIEW, payload: review }); +export type SetReviewActionType = {| + type: typeof SET_REVIEW, + payload: UserReviewType, +|}; + +const setReviewAction = ( + review: UserReviewType +): SetReviewActionType => ({ type: SET_REVIEW, payload: review }); -export const setReview = (review, reviewOverrides = {}) => { +export const setReview = ( + review: ApiReviewType, reviewOverrides: Object = {} +): SetReviewActionType => { if (!review) { throw new Error('review cannot be empty'); } + // $FLOW_FIXME: I think we can remove this by adjusting the tests return setReviewAction({ ...denormalizeReview(review), ...reviewOverrides, }); }; -export const setDenormalizedReview = (review) => { +export const setDenormalizedReview = (review: UserReviewType) => { if (!review) { throw new Error('review cannot be empty'); } return setReviewAction(review); }; -export const setAddonReviews = ({ addonSlug, reviews }) => { +export type SetAddonReviewsAction = {| + type: typeof SET_ADDON_REVIEWS, + payload: {| + addonSlug: string, + reviews: Array, + |}, +|}; + +type SetAddonReviewsParams = {| + addonSlug: string, + reviews: Array, +|}; + +export const setAddonReviews = ( + { addonSlug, reviews }: SetAddonReviewsParams +): SetAddonReviewsAction => { if (!addonSlug) { throw new Error('addonSlug cannot be empty'); } diff --git a/src/amo/api/index.js b/src/amo/api/index.js index 6ea1c3ae9a1..f512643a396 100644 --- a/src/amo/api/index.js +++ b/src/amo/api/index.js @@ -3,6 +3,27 @@ import { callApi } from 'core/api'; import { ErrorHandler } from 'core/errorHandler'; import log from 'core/logger'; +export type ApiReviewType = {| + addon: {| + id: number, + slug: string, + |}, + body: string, + created: Date, + title: string, + id: number, + is_latest: boolean, + rating: number, + user: {| + id: number, + name: string, + url: string, + |}, + version?: {| + id: number, + |}, +|}; + // TODO: make a separate function for posting/patching so that we // can type check each one independently. export type SubmitReviewParams = {| @@ -28,7 +49,7 @@ export function submitReview({ body, reviewId, ...apiCallParams -}: SubmitReviewParams): Promise { +}: SubmitReviewParams): Promise { return new Promise( (resolve) => { const data = { diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 2895923a13a..635ec015f0a 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -9,6 +9,7 @@ import type { UserReviewType } from 'amo/actions/reviews'; import { getLatestUserReview, submitReview } from 'amo/api'; import type { SubmitReviewParams } from 'amo/api'; import DefaultAddonReview from 'amo/components/AddonReview'; +import type { UrlFormatParams } from 'core/api'; import type { DispatchFn } from 'core/types/reduxTypes'; import type { AddonType, AddonTypeProp, AddonVersionType } from 'core/types/addonTypes'; @@ -39,10 +40,10 @@ type RatingManagerProps = {| Rating: typeof DefaultRating, addon: AddonType, errorHandler: typeof ErrorHandler, - apiState: Object, + apiState: Object, // TODO: apiState type from reducer i18n: Object, loadSavedReview: LoadSavedReviewFn, - location: Object, // might be builtin + location: UrlFormatParams, submitReview: SubmitReviewFn, userId: number, userReview: UserReviewType, diff --git a/src/core/actions/index.js b/src/core/actions/index.js index 487fddb679d..5b8fae179ca 100644 --- a/src/core/actions/index.js +++ b/src/core/actions/index.js @@ -1,3 +1,4 @@ +/* @flow */ import { ENTITIES_LOADED, LOG_OUT_USER, @@ -8,7 +9,12 @@ import { SET_USER_AGENT, } from 'core/constants'; -export function setAuthToken(token) { +export type SetAuthTokenAction = {| + type: typeof SET_AUTH_TOKEN, + payload: {| token: string |}, +|}; + +export function setAuthToken(token: string): SetAuthTokenAction { if (!token) { throw new Error('token cannot be falsey'); } @@ -18,11 +24,20 @@ export function setAuthToken(token) { }; } -export function logOutUser() { +export type LogOutUserAction = {| + type: typeof LOG_OUT_USER, +|}; + +export function logOutUser(): LogOutUserAction { return { type: LOG_OUT_USER }; } -export function setClientApp(clientApp) { +export type SetClientAppAction = {| + type: typeof SET_CLIENT_APP, + payload: {| clientApp: string |}, +|}; + +export function setClientApp(clientApp: string): SetClientAppAction { if (!clientApp) { throw new Error('clientApp cannot be falsey'); } @@ -32,28 +47,48 @@ export function setClientApp(clientApp) { }; } -export function setLang(lang) { +export type SetLangAction = {| + type: typeof SET_LANG, + payload: {| lang: string |}, +|}; + +export function setLang(lang: string): SetLangAction { return { type: SET_LANG, payload: { lang }, }; } -export function setUserAgent(userAgent) { +export type SetUserAgentAction = {| + type: typeof SET_USER_AGENT, + payload: {| userAgent: string |}, +|}; + +export function setUserAgent(userAgent: string): SetUserAgentAction { return { type: SET_USER_AGENT, payload: { userAgent }, }; } -export function loadEntities(entities) { +export type LoadEntitiesAction = {| + type: typeof ENTITIES_LOADED, + payload: {| entities: Array |}, +|}; + +export function loadEntities(entities: Array): LoadEntitiesAction { return { type: ENTITIES_LOADED, payload: { entities }, }; } -export function setCurrentUser(username) { +export type SetCurrentUserAction = {| + type: typeof SET_CURRENT_USER, + payload: {| username: string |}, +|}; + +export function setCurrentUser(username: string): SetCurrentUserAction { return { type: SET_CURRENT_USER, payload: { username }, diff --git a/src/core/api/index.js b/src/core/api/index.js index 72aae8e8418..036c2ddbafd 100644 --- a/src/core/api/index.js +++ b/src/core/api/index.js @@ -1,3 +1,4 @@ +/* @flow */ /* global fetch */ import url from 'url'; @@ -8,6 +9,9 @@ import { schema as normalizrSchema, normalize } from 'normalizr'; import { oneLine } from 'common-tags'; import config from 'config'; +import { ErrorHandler } from 'core/errorHandler'; +import { initialApiState } from 'core/reducers/api'; +import type { ApiStateType } from 'core/reducers/api'; import { ADDON_TYPE_THEME } from 'core/constants'; import log from 'core/logger'; import { convertFiltersToQueryParams } from 'core/searchUtils'; @@ -20,7 +24,7 @@ export const addon = new Entity('addons', {}, { idAttribute: 'slug' }); export const category = new Entity('categories', {}, { idAttribute: 'slug' }); export const user = new Entity('users', {}, { idAttribute: 'username' }); -export function makeQueryString(query) { +export function makeQueryString(query: { [key: string]: * }) { const resolvedQuery = { ...query }; Object.keys(resolvedQuery).forEach((key) => { const value = resolvedQuery[key]; @@ -33,7 +37,15 @@ export function makeQueryString(query) { return url.format({ query: resolvedQuery }); } -export function createApiError({ apiURL, response, jsonResponse }) { +type CreateApiErrorParams = {| + apiURL?: string, + response: { status: number }, + jsonResponse?: Object, +|}; + +export function createApiError( + { apiURL, response, jsonResponse }: CreateApiErrorParams +) { let urlId = '[unknown URL]'; if (apiURL) { // Strip the host since we already know that. @@ -42,6 +54,7 @@ export function createApiError({ apiURL, response, jsonResponse }) { urlId = urlId.split('?')[0]; } const apiError = new Error(`Error calling: ${urlId}`); + // $FLOW_FIXME: turn Error into a custom ApiError class. apiError.response = { apiURL, status: response.status, @@ -50,10 +63,29 @@ export function createApiError({ apiURL, response, jsonResponse }) { return apiError; } +type CallApiParams = {| + endpoint: string, + schema?: Object, + params?: Object, + auth?: boolean, + state?: ApiStateType, + method?: 'GET' | 'POST' | 'DELETE' | 'HEAD' | 'OPTIONS' | 'PUT' | 'PATCH', + body?: Object, + credentials?: boolean, + errorHandler?: typeof ErrorHandler, +|}; + export function callApi({ - endpoint, schema, params = {}, auth = false, state = {}, method = 'get', - body, credentials, errorHandler, -}) { + endpoint, + schema, + params = {}, + auth = false, + state = initialApiState, + method = 'GET', + body, + credentials, + errorHandler, +}: CallApiParams): Promise { if (errorHandler) { errorHandler.clear(); } @@ -63,6 +95,8 @@ export function callApi({ // Always make sure the method is upper case so that the browser won't // complain about CORS problems. method: method.toUpperCase(), + credentials: undefined, + body: undefined, }; if (credentials) { options.credentials = 'include'; @@ -79,6 +113,7 @@ export function callApi({ // Workaround for https://github.com/bitinn/node-fetch/issues/245 const apiURL = utf8.encode(`${API_BASE}/${endpoint}/${queryString}`); + // $FLOW_FIXME: once everything uses Flow we won't have to use toUpperCase return fetch(apiURL, options) .then((response) => { const contentType = response.headers.get('Content-Type').toLowerCase(); @@ -124,7 +159,16 @@ export function callApi({ .then((response) => (schema ? normalize(response, schema) : response)); } -export function search({ api, page, auth = false, filters = {} }) { +type SearchParams = {| + api: ApiStateType, + page: number, + auth: boolean, + filters: Object, +|}; + +export function search( + { api, page, auth = false, filters = {} }: SearchParams +) { const _filters = { ...filters }; if (!_filters.clientApp && api.clientApp) { log.debug( @@ -158,7 +202,12 @@ export function search({ api, page, auth = false, filters = {} }) { }); } -export function fetchAddon({ api, slug }) { +type FetchAddonParams = {| + api: ApiStateType, + slug: string, +|}; + +export function fetchAddon({ api, slug }: FetchAddonParams) { return callApi({ endpoint: `addons/addon/${slug}`, schema: addon, @@ -167,15 +216,21 @@ export function fetchAddon({ api, slug }) { }); } -export function login({ api, code, state }) { - const params = {}; +type LoginParams = {| + api: ApiStateType, + code: string, + state: string, +|}; + +export function login({ api, code, state }: LoginParams) { + const params = { config: undefined }; const configName = config.get('fxaConfig'); if (configName) { params.config = configName; } return callApi({ endpoint: 'accounts/login', - method: 'post', + method: 'POST', body: { code, state }, params, state: api, @@ -183,9 +238,27 @@ export function login({ api, code, state }) { }); } -export function startLoginUrl({ location }) { +// These are all possible parameters to url.format() +export type UrlFormatParams = {| + +href?: string; + +protocol?: string; + +slashes?: boolean; + +auth?: string; + +hostname?: string; + +port?: string | number; + +host?: string; + +pathname?: string; + +search?: string; + +query?: Object; + +hash?: string; +|}; + +export function startLoginUrl({ location }: { location: UrlFormatParams }) { const configName = config.get('fxaConfig'); - const params = { to: url.format({ ...location }) }; + const params = { + config: undefined, + to: url.format({ ...location }), + }; if (configName) { params.config = configName; } @@ -193,7 +266,7 @@ export function startLoginUrl({ location }) { return `${API_BASE}/accounts/login/start/${query}`; } -export function fetchProfile({ api }) { +export function fetchProfile({ api }: {| api: ApiStateType |}) { return callApi({ endpoint: 'accounts/profile', schema: user, @@ -202,7 +275,13 @@ export function fetchProfile({ api }) { }); } -export function featured({ api, filters, page }) { +type FeaturedParams = {| + api: ApiStateType, + filters: Object, + page: number, +|}; + +export function featured({ api, filters, page }: FeaturedParams) { return callApi({ endpoint: 'addons/featured', params: { @@ -215,7 +294,7 @@ export function featured({ api, filters, page }) { }); } -export function categories({ api }) { +export function categories({ api }: {| api: ApiStateType |}) { return callApi({ endpoint: 'addons/categories', schema: { results: [category] }, @@ -223,12 +302,12 @@ export function categories({ api }) { }); } -export function logOutFromServer({ api }) { +export function logOutFromServer({ api }: {| api: ApiStateType |}) { return callApi({ auth: true, credentials: true, endpoint: 'accounts/session', - method: 'delete', + method: 'DELETE', state: api, }); } diff --git a/src/core/components/AuthenticateButton/index.js b/src/core/components/AuthenticateButton/index.js index a576262182f..af76e78fa12 100644 --- a/src/core/components/AuthenticateButton/index.js +++ b/src/core/components/AuthenticateButton/index.js @@ -1,3 +1,4 @@ +/* @flow */ /* global window */ import React, { PropTypes } from 'react'; import { connect } from 'react-redux'; @@ -5,30 +6,33 @@ import { compose } from 'redux'; import { logOutUser } from 'core/actions'; import { logOutFromServer, startLoginUrl } from 'core/api'; +import type { UrlFormatParams } from 'core/api'; import translate from 'core/i18n/translate'; +import type { DispatchFn } from 'core/types/reduxTypes'; import Button from 'ui/components/Button'; import Icon from 'ui/components/Icon'; +type AuthenticateButtonProps = {| + api: Object, + className?: string, + handleLogIn: HandleLogInFn, + handleLogOut: HandleLogOutFn, + i18n: Object, + isAuthenticated: boolean, + location: UrlFormatParams, + logInText?: string, + logOutText?: string, + noIcon: boolean, +|}; export class AuthenticateButtonBase extends React.Component { - static propTypes = { - api: PropTypes.object.isRequired, - className: PropTypes.string, - handleLogIn: PropTypes.func.isRequired, - handleLogOut: PropTypes.func.isRequired, - i18n: PropTypes.object.isRequired, - isAuthenticated: PropTypes.bool.isRequired, - location: PropTypes.object.isRequired, - logInText: PropTypes.string, - logOutText: PropTypes.string, - noIcon: PropTypes.boolean, - } + props: AuthenticateButtonProps; static defaultProps = { noIcon: false, } - onClick = (event) => { + onClick = (event: Event) => { event.preventDefault(); event.stopPropagation(); const { @@ -57,7 +61,17 @@ export class AuthenticateButtonBase extends React.Component { } } -export const mapStateToProps = (state) => ({ +type HandleLogInFn = ( + location: UrlFormatParams, options?: {| _window: Object |} +) => void; + +type StateMappedProps = {| + api: Object, // TODO apiState from reducer + isAuthenticated: boolean, + handleLogIn: HandleLogInFn, +|}; + +export const mapStateToProps = (state: Object): StateMappedProps => ({ api: state.api, isAuthenticated: !!state.api.token, handleLogIn(location, { _window = window } = {}) { @@ -66,7 +80,17 @@ export const mapStateToProps = (state) => ({ }, }); -export const mapDispatchToProps = (dispatch) => ({ +type HandleLogOutFn = ({| + api: Object, // TODO: apiState from reducer +|}) => Promise; + +type DispatchMappedProps = {| + handleLogOut: HandleLogOutFn, +|}; + +export const mapDispatchToProps = ( + dispatch: DispatchFn +): DispatchMappedProps => ({ handleLogOut({ api }) { return logOutFromServer({ api }) .then(() => dispatch(logOutUser())); diff --git a/src/core/reducers/api.js b/src/core/reducers/api.js index 86af9b38823..b58a6d2eec3 100644 --- a/src/core/reducers/api.js +++ b/src/core/reducers/api.js @@ -1,5 +1,16 @@ +/* @flow */ import UAParser from 'ua-parser-js'; +import type { Exact } from 'core/types/coreTypes'; + +import type { + SetAuthTokenAction, + LogOutUserAction, + SetClientAppAction, + SetLangAction, + SetUserAgentAction, +} from 'core/actions/index'; + import { LOG_OUT_USER, SET_AUTH_TOKEN, @@ -8,7 +19,35 @@ import { SET_USER_AGENT, } from 'core/constants'; -export default function api(state = {}, action) { +type UserAgentInfoType = {| + browser: string, + os: string, +|}; + +export type ApiStateType = { + token: ?string, + lang: ?string, + clientApp: ?string, + userAgent: ?string, + userAgentInfo: ?UserAgentInfoType, +}; + +export const initialApiState = { + token: null, + lang: null, + clientApp: null, + userAgent: null, + userAgentInfo: null, +}; + +export default function api( + state: Exact = initialApiState, + action: SetAuthTokenAction + & SetLangAction + & SetClientAppAction + & SetUserAgentAction + & LogOutUserAction +): Exact { switch (action.type) { case SET_AUTH_TOKEN: return { ...state, token: action.payload.token }; @@ -27,11 +66,7 @@ export default function api(state = {}, action) { }; } case LOG_OUT_USER: - { - const newState = { ...state }; - delete newState.token; - return newState; - } + return { ...state, token: null }; default: return state; } diff --git a/src/core/types/coreTypes.js b/src/core/types/coreTypes.js new file mode 100644 index 00000000000..7a8db9b2739 --- /dev/null +++ b/src/core/types/coreTypes.js @@ -0,0 +1,5 @@ +/* @flow */ + +// This is a workaround for exact object types that supports es6 spreads. +// https://github.com/facebook/flow/issues/2405#issuecomment-274073091 +export type Exact = T & $Shape; From 560cc22e51ce31fb11cd743bed56ae885a1d9e54 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 22:26:56 +0000 Subject: [PATCH 10/71] added flow strip types --- .babelrc | 1 + package.json | 1 + yarn.lock | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.babelrc b/.babelrc index b86024af019..4a6d9d4dd7f 100644 --- a/.babelrc +++ b/.babelrc @@ -5,6 +5,7 @@ "react" ], "plugins": [ + "transform-flow-strip-types", "babel-plugin-dedent", "transform-class-properties", "transform-es2015-modules-commonjs" diff --git a/package.json b/package.json index a7c66e87b96..15cd8e79154 100644 --- a/package.json +++ b/package.json @@ -230,6 +230,7 @@ "babel-plugin-react-transform": "2.0.2", "babel-plugin-transform-class-properties": "6.18.0", "babel-plugin-transform-decorators-legacy": "1.3.4", + "babel-plugin-transform-flow-strip-types": "6.22.0", "babel-plugin-transform-object-rest-spread": "6.20.2", "babel-preset-es2015": "6.24.0", "babel-preset-react": "6.16.0", diff --git a/yarn.lock b/yarn.lock index 2a8d3e06dca..a0cd881bce3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -751,7 +751,7 @@ babel-plugin-transform-exponentiation-operator@^6.22.0: babel-plugin-syntax-exponentiation-operator "^6.8.0" babel-runtime "^6.22.0" -babel-plugin-transform-flow-strip-types@^6.3.13: +babel-plugin-transform-flow-strip-types@6.22.0, babel-plugin-transform-flow-strip-types@^6.3.13: version "6.22.0" resolved "https://registry.yarnpkg.com/babel-plugin-transform-flow-strip-types/-/babel-plugin-transform-flow-strip-types-6.22.0.tgz#84cb672935d43714fdc32bce84568d87441cf7cf" dependencies: From 6d666b06568b1ce9337183b272eb0bf0a8e5a487 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sat, 25 Mar 2017 22:31:42 +0000 Subject: [PATCH 11/71] make a separate ErrorHandler type --- src/amo/api/index.js | 4 ++-- src/amo/components/RatingManager/index.js | 5 +++-- src/core/api/index.js | 4 ++-- src/core/errorHandler.js | 2 ++ 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/amo/api/index.js b/src/amo/api/index.js index f512643a396..fa5bec3da95 100644 --- a/src/amo/api/index.js +++ b/src/amo/api/index.js @@ -1,6 +1,6 @@ /* @flow */ import { callApi } from 'core/api'; -import { ErrorHandler } from 'core/errorHandler'; +import type { ErrorHandlerType } from 'core/errorHandler'; import log from 'core/logger'; export type ApiReviewType = {| @@ -30,7 +30,7 @@ export type SubmitReviewParams = {| addonId?: number, rating?: number, apiState?: Object, - errorHandler?: typeof ErrorHandler, + errorHandler?: ErrorHandlerType, title?: string, versionId?: number, body?: string, diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 635ec015f0a..4493ac196a6 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -3,7 +3,8 @@ import React, { PropTypes } from 'react'; import { connect } from 'react-redux'; import { compose } from 'redux'; -import { ErrorHandler, withErrorHandling } from 'core/errorHandler'; +import { withErrorHandling } from 'core/errorHandler'; +import type { ErrorHandlerType } from 'core/errorHandler'; import { setReview } from 'amo/actions/reviews'; import type { UserReviewType } from 'amo/actions/reviews'; import { getLatestUserReview, submitReview } from 'amo/api'; @@ -39,7 +40,7 @@ type RatingManagerProps = {| AuthenticateButton: typeof DefaultAuthenticateButton, Rating: typeof DefaultRating, addon: AddonType, - errorHandler: typeof ErrorHandler, + errorHandler: ErrorHandlerType, apiState: Object, // TODO: apiState type from reducer i18n: Object, loadSavedReview: LoadSavedReviewFn, diff --git a/src/core/api/index.js b/src/core/api/index.js index 036c2ddbafd..f9204af6b42 100644 --- a/src/core/api/index.js +++ b/src/core/api/index.js @@ -9,7 +9,7 @@ import { schema as normalizrSchema, normalize } from 'normalizr'; import { oneLine } from 'common-tags'; import config from 'config'; -import { ErrorHandler } from 'core/errorHandler'; +import type { ErrorHandlerType } from 'core/errorHandler'; import { initialApiState } from 'core/reducers/api'; import type { ApiStateType } from 'core/reducers/api'; import { ADDON_TYPE_THEME } from 'core/constants'; @@ -72,7 +72,7 @@ type CallApiParams = {| method?: 'GET' | 'POST' | 'DELETE' | 'HEAD' | 'OPTIONS' | 'PUT' | 'PATCH', body?: Object, credentials?: boolean, - errorHandler?: typeof ErrorHandler, + errorHandler?: ErrorHandlerType, |}; export function callApi({ diff --git a/src/core/errorHandler.js b/src/core/errorHandler.js index 0884e7f0ae4..8c7b560211b 100644 --- a/src/core/errorHandler.js +++ b/src/core/errorHandler.js @@ -61,6 +61,8 @@ export class ErrorHandler { } } +export type ErrorHandlerType = typeof ErrorHandler; + /* * This is a decorator that gives a component the ability to handle errors. * From 4db137d912546e9ab9add944f8fd2de837b7d29a Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sun, 26 Mar 2017 14:50:41 -0500 Subject: [PATCH 12/71] fixing tests --- tests/client/core/api/test_api.js | 1 + tests/client/core/reducers/test_api.js | 11 ++++++----- tests/client/core/test_searchUtils.js | 5 +++-- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/tests/client/core/api/test_api.js b/tests/client/core/api/test_api.js index 8f962436139..b7656be3c99 100644 --- a/tests/client/core/api/test_api.js +++ b/tests/client/core/api/test_api.js @@ -587,6 +587,7 @@ describe('api', () => { const mockResponse = createApiResponse({ jsonData: { ok: true } }); mockWindow.expects('fetch') .withArgs(`${apiHost}/api/v3/accounts/session/?lang=en-US`, { + body: undefined, credentials: 'include', headers: { authorization: 'Bearer secret-token' }, method: 'DELETE', diff --git a/tests/client/core/reducers/test_api.js b/tests/client/core/reducers/test_api.js index 1fdac8817c2..8898e1c184e 100644 --- a/tests/client/core/reducers/test_api.js +++ b/tests/client/core/reducers/test_api.js @@ -1,7 +1,7 @@ import UAParser from 'ua-parser-js'; import * as actions from 'core/actions'; -import api from 'core/reducers/api'; +import api, { initialApiState } from 'core/reducers/api'; import { signedInApiState, userAgents, userAuthToken } from 'tests/client/helpers'; @@ -20,9 +20,9 @@ describe('api reducer', () => { }); it('clears the auth token on log out', () => { - const expectedState = { ...signedInApiState }; - assert.ok(expectedState.token, 'signed in state did not have a token'); - delete expectedState.token; + const state = { ...signedInApiState }; + assert.ok(state.token, 'signed in state did not have a token'); + const expectedState = { ...state, token: null }; assert.deepEqual( api(signedInApiState, actions.logOutUser()), expectedState); }); @@ -82,6 +82,7 @@ describe('api reducer', () => { }); it('defaults to an empty object', () => { - assert.deepEqual(api(undefined, { type: 'UNRELATED' }), {}); + assert.deepEqual( + api(undefined, { type: 'UNRELATED' }), { ...initialApiState }); }); }); diff --git a/tests/client/core/test_searchUtils.js b/tests/client/core/test_searchUtils.js index e4eb64de459..ebb89250904 100644 --- a/tests/client/core/test_searchUtils.js +++ b/tests/client/core/test_searchUtils.js @@ -1,6 +1,7 @@ import createStore from 'amo/store'; import * as searchActions from 'core/actions/search'; import * as api from 'core/api'; +import { initialApiState } from 'core/reducers/api'; import { ADDON_TYPE_THEME } from 'core/constants'; import { loadByCategoryIfNeeded, mapStateToProps } from 'core/searchUtils'; @@ -52,7 +53,7 @@ describe('searchUtils loadByCategoryIfNeeded()', () => { mockApi .expects('search') .once() - .withArgs({ page: 1, filters, api: {}, auth: {} }) + .withArgs({ page: 1, filters, api: { ...initialApiState }, auth: {} }) .returns(Promise.resolve({ entities, result })); return loadByCategoryIfNeeded({ store, @@ -77,7 +78,7 @@ describe('searchUtils loadByCategoryIfNeeded()', () => { mockApi .expects('search') .once() - .withArgs({ page: 1, filters, api: {}, auth: {} }) + .withArgs({ page: 1, filters, api: { ...initialApiState }, auth: {} }) .returns(Promise.resolve({ entities, result })); return loadByCategoryIfNeeded({ store, From 502543e1f53b7f59430193e34474027ce0067446 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sun, 26 Mar 2017 14:55:07 -0500 Subject: [PATCH 13/71] dedent comments --- flow/libs/dedent.js.flow | 2 ++ 1 file changed, 2 insertions(+) diff --git a/flow/libs/dedent.js.flow b/flow/libs/dedent.js.flow index 744c9b4cc94..8738e089e09 100644 --- a/flow/libs/dedent.js.flow +++ b/flow/libs/dedent.js.flow @@ -1 +1,3 @@ +// I'm not sure exactly what this should be. I was using this issue as a guide. +// https://github.com/facebook/flow/issues/2616#issuecomment-289257544 declare function dedent(params: Array<*>): string; From 1908d7fcb707ea0ada190a5c2627cf383cab6540 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sun, 26 Mar 2017 15:02:37 -0500 Subject: [PATCH 14/71] fixed location tests --- tests/client/core/api/test_api.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/client/core/api/test_api.js b/tests/client/core/api/test_api.js index b7656be3c99..f1776cae838 100644 --- a/tests/client/core/api/test_api.js +++ b/tests/client/core/api/test_api.js @@ -1,5 +1,6 @@ /* global Response, window */ import config from 'config'; +import querystring from 'querystring'; import utf8 from 'utf8'; import * as api from 'core/api'; @@ -539,19 +540,19 @@ describe('api', () => { }); describe('startLoginUrl', () => { + const getStartLoginQs = (location) => + querystring.parse(api.startLoginUrl({ location }).split('?')[1]); + it('includes the next path', () => { const location = { pathname: '/foo', query: { bar: 'BAR' } }; - assert.equal( - api.startLoginUrl({ location }), - `${apiHost}/api/v3/accounts/login/start/?to=%2Ffoo%3Fbar%3DBAR`); + assert.deepEqual(getStartLoginQs(location), { to: '/foo?bar=BAR' }); }); it('includes the next path the config if set', () => { sinon.stub(config, 'get').withArgs('fxaConfig').returns('my-config'); const location = { pathname: '/foo' }; - assert.equal( - api.startLoginUrl({ location }), - `${apiHost}/api/v3/accounts/login/start/?to=%2Ffoo&config=my-config`); + assert.deepEqual( + getStartLoginQs(location), { to: '/foo', config: 'my-config' }); }); }); From 83c7e87a3e7da8bafad559fb6fddeb6bb0cc72ef Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sun, 26 Mar 2017 15:27:56 -0500 Subject: [PATCH 15/71] fixed up more tests --- .../amo/components/TestAddonReviewList.js | 3 +- .../amo/components/TestRatingManager.js | 5 ++- tests/client/amo/testApi.js | 6 ++-- tests/client/amo/test_utils.js | 13 ++++--- tests/client/core/api/test_api.js | 35 ++++++++++++------- tests/client/helpers.js | 2 ++ 6 files changed, 42 insertions(+), 22 deletions(-) diff --git a/tests/client/amo/components/TestAddonReviewList.js b/tests/client/amo/components/TestAddonReviewList.js index c7bf90b2853..39a729c00de 100644 --- a/tests/client/amo/components/TestAddonReviewList.js +++ b/tests/client/amo/components/TestAddonReviewList.js @@ -21,6 +21,7 @@ import translate from 'core/i18n/translate'; import { loadEntities } from 'core/actions'; import * as coreApi from 'core/api'; import { denormalizeAddon } from 'core/reducers/addons'; +import { initialApiState } from 'core/reducers/api'; import I18nProvider from 'core/i18n/Provider'; import Rating from 'ui/components/Rating'; import { fakeAddon, fakeReview } from 'tests/client/amo/helpers'; @@ -254,7 +255,7 @@ describe('amo/components/AddonReviewList', () => { mockCoreApi .expects('fetchAddon') .once() - .withArgs({ slug: addonSlug, api: {} }) + .withArgs({ slug: addonSlug, api: { ...initialApiState } }) .returns(Promise.resolve(createFetchAddonResult(fakeAddon))); mockAmoApi diff --git a/tests/client/amo/components/TestRatingManager.js b/tests/client/amo/components/TestRatingManager.js index a21e77d0c90..e70753c6a52 100644 --- a/tests/client/amo/components/TestRatingManager.js +++ b/tests/client/amo/components/TestRatingManager.js @@ -14,6 +14,7 @@ import { ADDON_TYPE_THEME, } from 'core/constants'; import I18nProvider from 'core/i18n/Provider'; +import { initialApiState } from 'core/reducers/api'; import * as amoApi from 'amo/api'; import createStore from 'amo/store'; import { setReview } from 'amo/actions/reviews'; @@ -80,7 +81,6 @@ describe('RatingManager', () => { apiState: { ...signedInApiState, token: 'new-token' }, version: { id: 321 }, addon: { ...fakeAddon, id: 12345, slug: 'some-slug' }, - userId: 92345, }); return root.onSelectRating(5) .then(() => { @@ -91,7 +91,6 @@ describe('RatingManager', () => { assert.equal(call.apiState.token, 'new-token'); assert.equal(call.addonId, 12345); assert.equal(call.errorHandler, errorHandler); - assert.equal(call.userId, 92345); assert.strictEqual(call.reviewId, undefined); }); }); @@ -374,7 +373,7 @@ describe('RatingManager', () => { }); it('sets an empty apiState when not signed in', () => { - assert.deepEqual(getMappedProps().apiState, {}); + assert.deepEqual(getMappedProps().apiState, { ...initialApiState }); }); it('sets an empty userId when not signed in', () => { diff --git a/tests/client/amo/testApi.js b/tests/client/amo/testApi.js index 1ac724d6900..32371cce97e 100644 --- a/tests/client/amo/testApi.js +++ b/tests/client/amo/testApi.js @@ -4,6 +4,7 @@ import { submitReview, } from 'amo/api'; import * as api from 'core/api'; +import { initialApiState } from 'core/reducers/api'; import { unexpectedSuccess } from 'tests/client/helpers'; import { fakeReview, signedInApiState } from 'tests/client/amo/helpers'; @@ -18,10 +19,11 @@ describe('amo.api', () => { // These are all the default values for fields that can be posted to the // endpoint. const defaultParams = { - rating: undefined, - version: undefined, + addon: undefined, body: undefined, + rating: undefined, title: undefined, + version: undefined, }; const baseParams = { apiState: { ...signedInApiState, token: 'new-token' }, diff --git a/tests/client/amo/test_utils.js b/tests/client/amo/test_utils.js index 6c2415b2a8a..1ebb00f5000 100644 --- a/tests/client/amo/test_utils.js +++ b/tests/client/amo/test_utils.js @@ -5,6 +5,7 @@ import createStore from 'amo/store'; import * as featuredActions from 'amo/actions/featured'; import * as landingActions from 'amo/actions/landing'; import * as api from 'core/api'; +import { initialApiState } from 'core/reducers/api'; import { ADDON_TYPE_EXTENSION, ADDON_TYPE_THEME, @@ -39,7 +40,9 @@ describe('amo/utils', () => { mockApi .expects('featured') .once() - .withArgs({ api: {}, filters: { addonType, page_size: 25 } }) + .withArgs({ + api: { ...initialApiState }, filters: { addonType, page_size: 25 }, + }) .returns(Promise.resolve({ entities, result })); return loadFeaturedAddons({ store, params: ownProps.params }) @@ -61,13 +64,15 @@ describe('amo/utils', () => { mockApi .expects('featured') .once() - .withArgs({ api: {}, filters: { addonType, page_size: 4 } }) + .withArgs({ + api: { ...initialApiState }, filters: { addonType, page_size: 4 }, + }) .returns(Promise.resolve({ entities, result })); mockApi .expects('search') .once() .withArgs({ - api: {}, + api: { ...initialApiState }, filters: { addonType, page_size: 4, sort: SEARCH_SORT_TOP_RATED }, page: 1, }) @@ -76,7 +81,7 @@ describe('amo/utils', () => { .expects('search') .once() .withArgs({ - api: {}, + api: { ...initialApiState }, filters: { addonType, page_size: 4, sort: SEARCH_SORT_POPULAR }, page: 1, }) diff --git a/tests/client/core/api/test_api.js b/tests/client/core/api/test_api.js index f1776cae838..641b8b8610e 100644 --- a/tests/client/core/api/test_api.js +++ b/tests/client/core/api/test_api.js @@ -52,7 +52,7 @@ describe('api', () => { it('transforms method to upper case', () => { mockWindow.expects('fetch') .withArgs(`${apiHost}/api/v3/resource/`, { - method: 'GET', headers: {}, + body: undefined, credentials: undefined, method: 'GET', headers: {}, }) .once() .returns(createApiResponse()); @@ -64,7 +64,7 @@ describe('api', () => { const endpoint = 'diccionario-español-venezuela'; mockWindow.expects('fetch') .withArgs(utf8.encode(`${apiHost}/api/v3/${endpoint}/`), { - method: 'GET', headers: {}, + body: undefined, credentials: undefined, method: 'GET', headers: {}, }) .once() .returns(createApiResponse()); @@ -157,7 +157,7 @@ describe('api', () => { mockWindow.expects('fetch') .withArgs(`${apiHost}/api/v3/resource/`, { - method: 'GET', headers: {}, + body: undefined, credentials: undefined, method: 'GET', headers: {}, }) .once() .returns(response); @@ -396,9 +396,12 @@ describe('api', () => { it('sets the lang and slug', () => { mockWindow.expects('fetch') - .withArgs( - `${apiHost}/api/v3/addons/addon/foo/?lang=en-US`, - { headers: {}, method: 'GET' }) + .withArgs(`${apiHost}/api/v3/addons/addon/foo/?lang=en-US`, { + body: undefined, + credentials: undefined, + headers: {}, + method: 'GET', + }) .once() .returns(mockResponse()); return api.fetchAddon({ api: { lang: 'en-US' }, slug: 'foo' }) @@ -418,9 +421,12 @@ describe('api', () => { it('fails when the add-on is not found', () => { mockWindow .expects('fetch') - .withArgs( - `${apiHost}/api/v3/addons/addon/foo/?lang=en-US`, - { headers: {}, method: 'GET' }) + .withArgs(`${apiHost}/api/v3/addons/addon/foo/?lang=en-US`, { + body: undefined, + credentials: undefined, + headers: {}, + method: 'GET', + }) .once() .returns(mockResponse({ ok: false })); return api.fetchAddon({ api: { lang: 'en-US' }, slug: 'foo' }) @@ -435,9 +441,12 @@ describe('api', () => { const token = userAuthToken(); mockWindow .expects('fetch') - .withArgs( - `${apiHost}/api/v3/addons/addon/bar/?lang=en-US`, - { headers: { authorization: `Bearer ${token}` }, method: 'GET' }) + .withArgs(`${apiHost}/api/v3/addons/addon/bar/?lang=en-US`, { + body: undefined, + credentials: undefined, + headers: { authorization: `Bearer ${token}` }, + method: 'GET', + }) .once() .returns(mockResponse()); return api.fetchAddon({ api: { lang: 'en-US', token }, slug: 'bar' }) @@ -523,6 +532,8 @@ describe('api', () => { mockWindow .expects('fetch') .withArgs(`${apiHost}/api/v3/accounts/profile/?lang=en-US`, { + body: undefined, + credentials: undefined, headers: { authorization: `Bearer ${token}` }, method: 'GET', }) diff --git a/tests/client/helpers.js b/tests/client/helpers.js index 4ad868a6d7a..42314c6c1d0 100644 --- a/tests/client/helpers.js +++ b/tests/client/helpers.js @@ -7,6 +7,7 @@ import UAParser from 'ua-parser-js'; import { ADDON_TYPE_EXTENSION } from 'core/constants'; import { makeI18n } from 'core/i18n/utils'; +import { initialApiState } from 'core/reducers/api'; /* * Return a fake authentication token that can be @@ -109,6 +110,7 @@ export function assertNotHasClass(el, className) { const userAgentForState = 'Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.1'; const { browser, os } = UAParser(userAgentForState); export const signedInApiState = Object.freeze({ + ...initialApiState, lang: 'en-US', token: 'secret-token', userAgent: userAgentForState, From 89cd739e91e970e06648af0d682ca5582e04b6b9 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Sun, 26 Mar 2017 16:03:37 -0500 Subject: [PATCH 16/71] fixed setReview problems --- src/amo/actions/reviews.js | 22 +++---- .../amo/components/TestRatingManager.js | 18 +++--- tests/client/amo/reducers/testReviews.js | 60 +++++++++++++------ 3 files changed, 60 insertions(+), 40 deletions(-) diff --git a/src/amo/actions/reviews.js b/src/amo/actions/reviews.js index 5f988b866e4..c8ce719498f 100644 --- a/src/amo/actions/reviews.js +++ b/src/amo/actions/reviews.js @@ -35,33 +35,25 @@ export function denormalizeReview(review: Object): UserReviewType { }; } -export type SetReviewActionType = {| +export type SetReviewAction = {| type: typeof SET_REVIEW, payload: UserReviewType, |}; -const setReviewAction = ( - review: UserReviewType -): SetReviewActionType => ({ type: SET_REVIEW, payload: review }); - -export const setReview = ( - review: ApiReviewType, reviewOverrides: Object = {} -): SetReviewActionType => { +export const setReview = (review: ApiReviewType): SetReviewAction => { if (!review) { throw new Error('review cannot be empty'); } - // $FLOW_FIXME: I think we can remove this by adjusting the tests - return setReviewAction({ - ...denormalizeReview(review), - ...reviewOverrides, - }); + return { type: SET_REVIEW, payload: denormalizeReview(review) }; }; -export const setDenormalizedReview = (review: UserReviewType) => { +export const setDenormalizedReview = ( + review: UserReviewType +): SetReviewAction => { if (!review) { throw new Error('review cannot be empty'); } - return setReviewAction(review); + return { type: SET_REVIEW, payload: review }; }; export type SetAddonReviewsAction = {| diff --git a/tests/client/amo/components/TestRatingManager.js b/tests/client/amo/components/TestRatingManager.js index e70753c6a52..9dc4504586b 100644 --- a/tests/client/amo/components/TestRatingManager.js +++ b/tests/client/amo/components/TestRatingManager.js @@ -394,7 +394,7 @@ describe('RatingManager', () => { it('sets a user review to the latest matching one in state', () => { signIn({ userId: fakeReview.user.id }); - const action = setReview(fakeReview, { isLatest: true }); + const action = setReview({ ...fakeReview, is_latest: true }); store.dispatch(action); const dispatchedReview = action.payload; @@ -410,9 +410,13 @@ describe('RatingManager', () => { signIn({ userId: userIdOne }); // Save a review for user two. - store.dispatch(setReview(fakeReview, { - isLatest: true, - userId: userIdTwo, + store.dispatch(setReview({ + ...fakeReview, + is_latest: true, + user: { + ...fakeReview.user, + id: userIdTwo, + }, rating: savedRating, })); @@ -434,18 +438,18 @@ describe('RatingManager', () => { signIn({ userId: fakeReview.user.id }); function createReview(overrides) { - const action = setReview(fakeReview, overrides); + const action = setReview({ ...fakeReview, ...overrides }); store.dispatch(action); return action.payload; } createReview({ id: 1, - isLatest: false, + is_latest: false, }); const latestReview = createReview({ id: 2, - isLatest: true, + is_latest: true, }); assert.deepEqual(getMappedProps().userReview, latestReview); diff --git a/tests/client/amo/reducers/testReviews.js b/tests/client/amo/reducers/testReviews.js index 6dbd6bf3ecd..8c446d3ac5f 100644 --- a/tests/client/amo/reducers/testReviews.js +++ b/tests/client/amo/reducers/testReviews.js @@ -3,13 +3,37 @@ import reviews, { initialState } from 'amo/reducers/reviews'; import { fakeAddon, fakeReview } from 'tests/client/amo/helpers'; describe('amo.reducers.reviews', () => { + function setFakeReview({ + userId = fakeReview.user.id, + addonId = fakeReview.addon.id, + versionId = fakeReview.version.id, + ...overrides } = {} + ) { + return setReview({ + ...fakeReview, + user: { + ...fakeReview.user, + id: userId, + }, + addon: { + ...fakeReview.addon, + id: addonId, + }, + version: { + ...fakeReview.version, + id: versionId, + }, + ...overrides, + }); + } + it('defaults to an empty object', () => { assert.deepEqual(reviews(undefined, { type: 'SOME_OTHER_ACTION' }), initialState); }); it('stores a user review', () => { - const action = setReview(fakeReview); + const action = setFakeReview(); const state = reviews(undefined, action); const storedReview = state[fakeReview.user.id][fakeReview.addon.id][fakeReview.id]; @@ -32,21 +56,21 @@ describe('amo.reducers.reviews', () => { it('preserves existing user rating data', () => { let state; - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 1, userId: 1, addonId: 1, rating: 1, })); - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 2, userId: 1, addonId: 2, rating: 5, })); - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 3, userId: 2, addonId: 2, @@ -64,17 +88,17 @@ describe('amo.reducers.reviews', () => { const userId = fakeReview.user.id; const addonId = fakeReview.addon.id; - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 1, versionId: 1, })); - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 2, versionId: 2, })); - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 3, versionId: 3, })); @@ -87,7 +111,7 @@ describe('amo.reducers.reviews', () => { it('preserves unrelated state', () => { let state = { ...initialState, somethingUnrelated: 'erp' }; - state = reviews(state, setReview(fakeReview)); + state = reviews(state, setFakeReview()); assert.equal(state.somethingUnrelated, 'erp'); }); @@ -96,19 +120,19 @@ describe('amo.reducers.reviews', () => { const userId = fakeReview.user.id; let state; - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 1, - isLatest: true, + is_latest: true, })); - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 2, - isLatest: true, + is_latest: true, })); - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 3, - isLatest: true, + is_latest: true, })); // Make sure only the newest submitted one is the latest: @@ -122,14 +146,14 @@ describe('amo.reducers.reviews', () => { const userId = fakeReview.user.id; let state; - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 1, - isLatest: true, + is_latest: true, })); - state = reviews(state, setReview(fakeReview, { + state = reviews(state, setFakeReview({ id: 2, - isLatest: false, + is_latest: false, })); assert.equal(state[userId][addonId][1].isLatest, true); From ce4a39d453cb7f7340c524eb5a5a7e88f93b7817 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 09:53:41 -0500 Subject: [PATCH 17/71] added missing api state types --- src/amo/api/index.js | 3 ++- src/amo/components/RatingManager/index.js | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/amo/api/index.js b/src/amo/api/index.js index fa5bec3da95..7f21526fc06 100644 --- a/src/amo/api/index.js +++ b/src/amo/api/index.js @@ -1,5 +1,6 @@ /* @flow */ import { callApi } from 'core/api'; +import type { ApiStateType } from 'core/reducers/api'; import type { ErrorHandlerType } from 'core/errorHandler'; import log from 'core/logger'; @@ -29,7 +30,7 @@ export type ApiReviewType = {| export type SubmitReviewParams = {| addonId?: number, rating?: number, - apiState?: Object, + apiState?: ApiStateType, errorHandler?: ErrorHandlerType, title?: string, versionId?: number, diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 4493ac196a6..1afcb1ec796 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -11,6 +11,7 @@ import { getLatestUserReview, submitReview } from 'amo/api'; import type { SubmitReviewParams } from 'amo/api'; import DefaultAddonReview from 'amo/components/AddonReview'; import type { UrlFormatParams } from 'core/api'; +import type { ApiStateType } from 'core/reducers/api'; import type { DispatchFn } from 'core/types/reduxTypes'; import type { AddonType, AddonTypeProp, AddonVersionType } from 'core/types/addonTypes'; @@ -41,7 +42,7 @@ type RatingManagerProps = {| Rating: typeof DefaultRating, addon: AddonType, errorHandler: ErrorHandlerType, - apiState: Object, // TODO: apiState type from reducer + apiState: ApiStateType, i18n: Object, loadSavedReview: LoadSavedReviewFn, location: UrlFormatParams, From b935fd78f555b28eeb617f12542d8d85ad4b97f3 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 11:10:13 -0500 Subject: [PATCH 18/71] Add minimal typing to i18n/utils --- src/core/i18n/utils.js | 53 +++++++++++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/src/core/i18n/utils.js b/src/core/i18n/utils.js index 3617d7dd45a..36daf01c6bb 100644 --- a/src/core/i18n/utils.js +++ b/src/core/i18n/utils.js @@ -1,3 +1,4 @@ +/* @flow */ import config from 'config'; import Jed from 'jed'; import moment from 'moment'; @@ -13,7 +14,7 @@ const supportedLangs = langs.concat(Object.keys(langMap)); const rtlLangs = config.get('rtlLangs'); -export function localeToLang(locale, log_ = log) { +export function localeToLang(locale?: any, log_?: typeof log = log) { let lang; if (locale && locale.split) { const parts = locale.split('_'); @@ -34,7 +35,7 @@ export function localeToLang(locale, log_ = log) { return lang; } -export function langToLocale(language, log_ = log) { +export function langToLocale(language?: any, log_?: typeof log = log) { let locale; if (language && language.split) { const parts = language.split('-'); @@ -55,23 +56,36 @@ export function langToLocale(language, log_ = log) { return locale; } -export function normalizeLang(lang) { +export function normalizeLang(lang?: string) { return localeToLang(langToLocale(lang)); } -export function normalizeLocale(locale) { +export function normalizeLocale(locale: string) { return langToLocale(localeToLang(locale)); } -export function isSupportedLang(lang, { _supportedLangs = supportedLangs } = {}) { +type IsSupportedLangOptions = {| + _supportedLangs: typeof supportedLangs, +|}; + +export function isSupportedLang( + lang?: string, + { _supportedLangs = supportedLangs }: IsSupportedLangOptions = {} +) { return _supportedLangs.includes(lang); } -export function isValidLang(lang, { _langs = langs } = {}) { +type IsValidLangOptions = {| + _langs: typeof langs, +|}; + +export function isValidLang( + lang?: string, { _langs = langs }: IsValidLangOptions = {} +) { return _langs.includes(lang); } -export function sanitizeLanguage(langOrLocale) { +export function sanitizeLanguage(langOrLocale?: string) { let language = normalizeLang(langOrLocale); // Only look in the un-mapped lang list. if (!isValidLang(language)) { @@ -81,12 +95,12 @@ export function sanitizeLanguage(langOrLocale) { return language; } -export function isRtlLang(lang) { +export function isRtlLang(lang: string) { const language = sanitizeLanguage(lang); return rtlLangs.includes(language); } -export function getDirection(lang) { +export function getDirection(lang: string) { return isRtlLang(lang) ? 'rtl' : 'ltr'; } @@ -105,7 +119,7 @@ function qualityCmp(a, b) { * sorted array of objects. Example object: * { lang: 'pl', quality: 0.7 } */ -export function parseAcceptLanguage(header) { +export function parseAcceptLanguage(header: string) { // pl,fr-FR;q=0.3,en-US;q=0.1 if (!header || !header.split) { return []; @@ -129,6 +143,10 @@ export function parseAcceptLanguage(header) { return langList; } +type GetLangFromHeaderOptions = {| + _supportedLangs?: Object, +|}; + /* * Given an accept-language header and a list of currently @@ -137,7 +155,9 @@ export function parseAcceptLanguage(header) { * Note: this doesn't map languages e.g. pt -> pt-PT. Use sanitizeLanguage for that. * */ -export function getLangFromHeader(acceptLanguage, { _supportedLangs } = {}) { +export function getLangFromHeader( + acceptLanguage: string, { _supportedLangs }: GetLangFromHeaderOptions = {} +) { let userLang; if (acceptLanguage) { const langList = parseAcceptLanguage(acceptLanguage); @@ -156,13 +176,18 @@ export function getLangFromHeader(acceptLanguage, { _supportedLangs } = {}) { return normalizeLang(userLang); } +type GetLanguageParams = {| + lang: string, + acceptLanguage: string, +|}; + /* * Check validity of language: * - If invalid, fall-back to accept-language. * - Return object with lang and isLangFromHeader hint. * */ -export function getLanguage({ lang, acceptLanguage } = {}) { +export function getLanguage({ lang, acceptLanguage }: GetLanguageParams) { let userLang = lang; let isLangFromHeader = false; // If we don't have a supported userLang yet try accept-language. @@ -177,7 +202,7 @@ export function getLanguage({ lang, acceptLanguage } = {}) { } // moment uses locales like "en-gb" whereas we use "en_GB". -export function makeMomentLocale(locale) { +export function makeMomentLocale(locale: string) { return locale.replace('_', '-').toLowerCase(); } @@ -193,7 +218,7 @@ function oneLineTranslationString(translationKey) { // Create an i18n object with a translated moment object available we can // use for translated dates across the app. -export function makeI18n(i18nData, lang, _Jed = Jed) { +export function makeI18n(i18nData: Object, lang: string, _Jed: Jed = Jed) { const i18n = new _Jed(i18nData); i18n.lang = lang; From bd242ea99b3b6b9401ea966be2ecded698204cfe Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 11:14:23 -0500 Subject: [PATCH 19/71] fix i18n optional keywords --- src/core/i18n/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/i18n/utils.js b/src/core/i18n/utils.js index 36daf01c6bb..6a79375104c 100644 --- a/src/core/i18n/utils.js +++ b/src/core/i18n/utils.js @@ -187,7 +187,7 @@ type GetLanguageParams = {| * - Return object with lang and isLangFromHeader hint. * */ -export function getLanguage({ lang, acceptLanguage }: GetLanguageParams) { +export function getLanguage({ lang, acceptLanguage }: GetLanguageParams = {}) { let userLang = lang; let isLangFromHeader = false; // If we don't have a supported userLang yet try accept-language. From 21e9c5bacc0c2911f235ec40d3f798b55bd33c65 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 12:59:12 -0500 Subject: [PATCH 20/71] type hint for the i18n config --- src/core/i18n/utils.js | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/core/i18n/utils.js b/src/core/i18n/utils.js index 6a79375104c..ab9ef61f4ac 100644 --- a/src/core/i18n/utils.js +++ b/src/core/i18n/utils.js @@ -216,12 +216,32 @@ function oneLineTranslationString(translationKey) { return translationKey; } +type I18nConfig = {| + // The following keys configure Jed. + // See http://messageformat.github.io/Jed/ + domain: string, + locale_data: { + [domain: string]: { + '': { // an empty string configures the domain. + domain: string, + lang: string, + plural_forms: string, + }, + [message: string]: Array, + }, + }, + // This is our custom configuration for moment. + _momentDefineLocale?: Function, +|}; + // Create an i18n object with a translated moment object available we can // use for translated dates across the app. -export function makeI18n(i18nData: Object, lang: string, _Jed: Jed = Jed) { +export function makeI18n(i18nData: I18nConfig, lang: string, _Jed: Jed = Jed) { const i18n = new _Jed(i18nData); i18n.lang = lang; + // TODO: move all of this to an I18n class that extends Jed so that we + // can type-check all the components that rely on the i18n object. i18n.formatNumber = (number) => number.toLocaleString(lang); // This adds the correct moment locale for the active locale so we can get From 3f4d74e1bbcc2e40727cab396e13d52ed6ad958a Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 13:26:34 -0500 Subject: [PATCH 21/71] fixed lint errors --- src/amo/api/index.js | 2 +- src/amo/components/RatingManager/index.js | 50 +++++++++---------- src/core/api/index.js | 6 ++- .../components/AuthenticateButton/index.js | 27 +++++----- src/core/reducers/api.js | 18 +++---- src/core/types/addonTypes.js | 1 - src/core/types/coreTypes.js | 3 +- tests/client/amo/testApi.js | 1 - tests/client/core/api/test_api.js | 3 +- 9 files changed, 57 insertions(+), 54 deletions(-) diff --git a/src/amo/api/index.js b/src/amo/api/index.js index 7f21526fc06..c04c13b9c3b 100644 --- a/src/amo/api/index.js +++ b/src/amo/api/index.js @@ -58,7 +58,7 @@ export function submitReview({ rating, version: versionId, body, - title + title, }; let method = 'POST'; let endpoint = 'reviews/review'; diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 1afcb1ec796..17d1467e05e 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -1,20 +1,14 @@ /* @flow */ -import React, { PropTypes } from 'react'; +/* global Node */ +/* eslint-disable react/sort-comp, react/no-unused-prop-types */ +import React from 'react'; import { connect } from 'react-redux'; import { compose } from 'redux'; import { withErrorHandling } from 'core/errorHandler'; -import type { ErrorHandlerType } from 'core/errorHandler'; import { setReview } from 'amo/actions/reviews'; -import type { UserReviewType } from 'amo/actions/reviews'; import { getLatestUserReview, submitReview } from 'amo/api'; -import type { SubmitReviewParams } from 'amo/api'; import DefaultAddonReview from 'amo/components/AddonReview'; -import type { UrlFormatParams } from 'core/api'; -import type { ApiStateType } from 'core/reducers/api'; -import type { DispatchFn } from 'core/types/reduxTypes'; -import type { AddonType, AddonTypeProp, AddonVersionType } - from 'core/types/addonTypes'; import DefaultAuthenticateButton from 'core/components/AuthenticateButton'; import { ADDON_TYPE_DICT, @@ -27,14 +21,25 @@ import { import translate from 'core/i18n/translate'; import log from 'core/logger'; import DefaultRating from 'ui/components/Rating'; +/* eslint-disable no-duplicate-imports */ +import type { ErrorHandlerType } from 'core/errorHandler'; +import type { UserReviewType } from 'amo/actions/reviews'; +import type { SubmitReviewParams } from 'amo/api'; +import type { UrlFormatParams } from 'core/api'; +import type { ApiStateType } from 'core/reducers/api'; +import type { DispatchFn } from 'core/types/reduxTypes'; +import type { AddonType, AddonTypeProp, AddonVersionType } + from 'core/types/addonTypes'; +/* eslint-enable no-duplicate-imports */ import './styles.scss'; -type GetLogInPromptParams = {| addonType: AddonTypeProp |}; +type LoadSavedReviewFn = ({| + userId: number, + addonId: number, +|}) => Promise<*>; -type GetLogInPromptOptions = {| - validAddonTypes: typeof defaultValidAddonTypes, -|}; +type SubmitReviewFn = (SubmitReviewParams) => Promise; type RatingManagerProps = {| AddonReview: typeof DefaultAddonReview, @@ -55,9 +60,7 @@ type RatingManagerProps = {| export class RatingManagerBase extends React.Component { props: RatingManagerProps; ratingLegend: Node; - state: {| - showTextEntry: boolean, - |}; + state: {| showTextEntry: boolean |}; static defaultProps = { AddonReview: DefaultAddonReview, @@ -109,8 +112,12 @@ export class RatingManagerBase extends React.Component { } getLogInPrompt( - { addonType }: GetLogInPromptParams, - { validAddonTypes = defaultValidAddonTypes }: GetLogInPromptOptions = {} + { addonType }: {| addonType: AddonTypeProp |}, + { + validAddonTypes = defaultValidAddonTypes, + }: {| + validAddonTypes: typeof defaultValidAddonTypes, + |} = {} ) { const { i18n } = this.props; switch (addonType) { @@ -216,13 +223,6 @@ export const mapStateToProps = ( }; }; -type LoadSavedReviewFn = ({| - userId: number, - addonId: number, -|}) => Promise<*>; - -type SubmitReviewFn = (SubmitReviewParams) => Promise; - type DispatchMappedProps = {| loadSavedReview: LoadSavedReviewFn, submitReview: SubmitReviewFn, diff --git a/src/core/api/index.js b/src/core/api/index.js index f9204af6b42..b28e2da9af5 100644 --- a/src/core/api/index.js +++ b/src/core/api/index.js @@ -9,12 +9,14 @@ import { schema as normalizrSchema, normalize } from 'normalizr'; import { oneLine } from 'common-tags'; import config from 'config'; -import type { ErrorHandlerType } from 'core/errorHandler'; import { initialApiState } from 'core/reducers/api'; -import type { ApiStateType } from 'core/reducers/api'; import { ADDON_TYPE_THEME } from 'core/constants'; import log from 'core/logger'; import { convertFiltersToQueryParams } from 'core/searchUtils'; +/* eslint-disable no-duplicate-imports */ +import type { ErrorHandlerType } from 'core/errorHandler'; +import type { ApiStateType } from 'core/reducers/api'; +/* eslint-enable no-duplicate-imports */ const API_BASE = `${config.get('apiHost')}${config.get('apiPath')}`; diff --git a/src/core/components/AuthenticateButton/index.js b/src/core/components/AuthenticateButton/index.js index af76e78fa12..7e27a86a5a2 100644 --- a/src/core/components/AuthenticateButton/index.js +++ b/src/core/components/AuthenticateButton/index.js @@ -1,16 +1,27 @@ /* @flow */ -/* global window */ -import React, { PropTypes } from 'react'; +/* global Event, window */ +/* eslint-disable react/sort-comp */ +import React from 'react'; import { connect } from 'react-redux'; import { compose } from 'redux'; import { logOutUser } from 'core/actions'; import { logOutFromServer, startLoginUrl } from 'core/api'; -import type { UrlFormatParams } from 'core/api'; import translate from 'core/i18n/translate'; -import type { DispatchFn } from 'core/types/reduxTypes'; import Button from 'ui/components/Button'; import Icon from 'ui/components/Icon'; +/* eslint-disable no-duplicate-imports */ +import type { UrlFormatParams } from 'core/api'; +import type { DispatchFn } from 'core/types/reduxTypes'; +/* eslint-enable no-duplicate-imports */ + +type HandleLogInFn = ( + location: UrlFormatParams, options?: {| _window: typeof window |} +) => void; + +type HandleLogOutFn = ({| + api: Object, // TODO: apiState from reducer +|}) => Promise; type AuthenticateButtonProps = {| api: Object, @@ -61,10 +72,6 @@ export class AuthenticateButtonBase extends React.Component { } } -type HandleLogInFn = ( - location: UrlFormatParams, options?: {| _window: Object |} -) => void; - type StateMappedProps = {| api: Object, // TODO apiState from reducer isAuthenticated: boolean, @@ -80,10 +87,6 @@ export const mapStateToProps = (state: Object): StateMappedProps => ({ }, }); -type HandleLogOutFn = ({| - api: Object, // TODO: apiState from reducer -|}) => Promise; - type DispatchMappedProps = {| handleLogOut: HandleLogOutFn, |}; diff --git a/src/core/reducers/api.js b/src/core/reducers/api.js index b58a6d2eec3..e95b8440f0a 100644 --- a/src/core/reducers/api.js +++ b/src/core/reducers/api.js @@ -1,16 +1,6 @@ /* @flow */ import UAParser from 'ua-parser-js'; -import type { Exact } from 'core/types/coreTypes'; - -import type { - SetAuthTokenAction, - LogOutUserAction, - SetClientAppAction, - SetLangAction, - SetUserAgentAction, -} from 'core/actions/index'; - import { LOG_OUT_USER, SET_AUTH_TOKEN, @@ -18,6 +8,14 @@ import { SET_CLIENT_APP, SET_USER_AGENT, } from 'core/constants'; +import type { + SetAuthTokenAction, + LogOutUserAction, + SetClientAppAction, + SetLangAction, + SetUserAgentAction, +} from 'core/actions/index'; +import type { Exact } from 'core/types/coreTypes'; type UserAgentInfoType = {| browser: string, diff --git a/src/core/types/addonTypes.js b/src/core/types/addonTypes.js index c767796064c..3c8df72e67e 100644 --- a/src/core/types/addonTypes.js +++ b/src/core/types/addonTypes.js @@ -5,7 +5,6 @@ import { ADDON_TYPE_LANG, ADDON_TYPE_SEARCH, ADDON_TYPE_THEME, - validAddonTypes as defaultValidAddonTypes, } from 'core/constants'; // TODO: Are all these fields actually in addon.current_version diff --git a/src/core/types/coreTypes.js b/src/core/types/coreTypes.js index 7a8db9b2739..1c63c17ab79 100644 --- a/src/core/types/coreTypes.js +++ b/src/core/types/coreTypes.js @@ -1,5 +1,6 @@ /* @flow */ +/* global $Shape */ -// This is a workaround for exact object types that supports es6 spreads. +// This is a workaround for exact object types that support object spreads. // https://github.com/facebook/flow/issues/2405#issuecomment-274073091 export type Exact = T & $Shape; diff --git a/tests/client/amo/testApi.js b/tests/client/amo/testApi.js index 32371cce97e..cc87d304aa2 100644 --- a/tests/client/amo/testApi.js +++ b/tests/client/amo/testApi.js @@ -4,7 +4,6 @@ import { submitReview, } from 'amo/api'; import * as api from 'core/api'; -import { initialApiState } from 'core/reducers/api'; import { unexpectedSuccess } from 'tests/client/helpers'; import { fakeReview, signedInApiState } from 'tests/client/amo/helpers'; diff --git a/tests/client/core/api/test_api.js b/tests/client/core/api/test_api.js index 641b8b8610e..3ba9896a4ac 100644 --- a/tests/client/core/api/test_api.js +++ b/tests/client/core/api/test_api.js @@ -1,6 +1,7 @@ /* global Response, window */ -import config from 'config'; import querystring from 'querystring'; + +import config from 'config'; import utf8 from 'utf8'; import * as api from 'core/api'; From eee97ff69cd9b67d01bccf67befdfa02942b9d0c Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 13:28:26 -0500 Subject: [PATCH 22/71] added missing ApiStateType --- src/core/components/AuthenticateButton/index.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/core/components/AuthenticateButton/index.js b/src/core/components/AuthenticateButton/index.js index 7e27a86a5a2..b543b17bac9 100644 --- a/src/core/components/AuthenticateButton/index.js +++ b/src/core/components/AuthenticateButton/index.js @@ -12,6 +12,7 @@ import Button from 'ui/components/Button'; import Icon from 'ui/components/Icon'; /* eslint-disable no-duplicate-imports */ import type { UrlFormatParams } from 'core/api'; +import type { ApiStateType } from 'core/reducers/api'; import type { DispatchFn } from 'core/types/reduxTypes'; /* eslint-enable no-duplicate-imports */ @@ -19,9 +20,7 @@ type HandleLogInFn = ( location: UrlFormatParams, options?: {| _window: typeof window |} ) => void; -type HandleLogOutFn = ({| - api: Object, // TODO: apiState from reducer -|}) => Promise; +type HandleLogOutFn = ({| api: ApiStateType |}) => Promise; type AuthenticateButtonProps = {| api: Object, From 92be514308436f79d2f76b1cbac185ef274c12c1 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 13:30:10 -0500 Subject: [PATCH 23/71] fixed another missing ApiStateType --- src/core/components/AuthenticateButton/index.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/components/AuthenticateButton/index.js b/src/core/components/AuthenticateButton/index.js index b543b17bac9..cf5969892cc 100644 --- a/src/core/components/AuthenticateButton/index.js +++ b/src/core/components/AuthenticateButton/index.js @@ -72,12 +72,14 @@ export class AuthenticateButtonBase extends React.Component { } type StateMappedProps = {| - api: Object, // TODO apiState from reducer + api: ApiStateType, isAuthenticated: boolean, handleLogIn: HandleLogInFn, |}; -export const mapStateToProps = (state: Object): StateMappedProps => ({ +export const mapStateToProps = ( + state: {| api: ApiStateType |} +): StateMappedProps => ({ api: state.api, isAuthenticated: !!state.api.token, handleLogIn(location, { _window = window } = {}) { From 86be2fb621b9fc6183955a32e505221c7b72744d Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 13:31:48 -0500 Subject: [PATCH 24/71] added todo comment --- src/amo/components/RatingManager/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 17d1467e05e..34c0804c098 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -193,6 +193,7 @@ export class RatingManagerBase extends React.Component { } } +// TODO: when all state types are exported, define `state`. export const mapStateToProps = ( state: Object, ownProps: RatingManagerProps ) => { From 09e2570239e0923852a06ee91587248363f1e307 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 13:45:56 -0500 Subject: [PATCH 25/71] complete Addon type --- src/core/types/addonTypes.js | 56 ++++++++++++++++++++++++++++-------- 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/src/core/types/addonTypes.js b/src/core/types/addonTypes.js index 3c8df72e67e..bcddbeb3d4b 100644 --- a/src/core/types/addonTypes.js +++ b/src/core/types/addonTypes.js @@ -7,12 +7,15 @@ import { ADDON_TYPE_THEME, } from 'core/constants'; -// TODO: Are all these fields actually in addon.current_version export type AddonVersionType = {| id: number, + channel: string, + edit_url: string, + files: Array, + // The `text` property is omitted from addon.current_version.license. license: { name: string, url: string }, + reviewed: Date, version: string, - files: Array, |}; export type AddonAuthorType = {| @@ -29,22 +32,51 @@ export type AddonTypeProp = export type AddonType = {| id: number, - guid: string, - name: string, - icon_url: string, - slug: string, - average_daily_users: number, authors: Array, + average_daily_users: number, + categories: Object, + compatibility: Object, current_version: AddonVersionType, + default_locale: string, + description: string, + edit_url: string, + guid: string, + has_eula: boolean, + has_privacy_policy: boolean, + homepage: string, + icon_url: string, + is_disabled: boolean, + is_experimental: boolean, + is_source_public: boolean, + name: string, + last_updated: Date, + latest_unlisted_version: ?AddonVersionType, previews: Array, - ratings: { + public_stats: boolean, + ratings: {| count: number, average: number, - }, + |}, + review_url: string, + slug: string, + status: 'beta' + | 'lite' + | 'public' + | 'deleted' + | 'pending' + | 'disabled' + | 'rejected' + | 'nominated' + | 'incomplete' + | 'unreviewed' + | 'lite-nominated' + | 'review-pending', summary: string, - description: string, - has_privacy_policy: boolean, - homepage: string, + support_email: string, support_url: string, + tags: Array, + theme_data: Object, type: AddonTypeProp, + url: string, + weekly_downloads: number, |}; From cfb67b0f21a30cd5ef4c571d00e0746013e8d508 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 13:51:03 -0500 Subject: [PATCH 26/71] fixed missing review type --- src/amo/actions/reviews.js | 4 ++-- src/amo/api/index.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/amo/actions/reviews.js b/src/amo/actions/reviews.js index c8ce719498f..2e8bce879db 100644 --- a/src/amo/actions/reviews.js +++ b/src/amo/actions/reviews.js @@ -14,10 +14,10 @@ export type UserReviewType = {| userId: number, userName: string, userUrl: string, - versionId: number, + versionId: ?number, |}; -export function denormalizeReview(review: Object): UserReviewType { +export function denormalizeReview(review: ApiReviewType): UserReviewType { return { addonId: review.addon.id, addonSlug: review.addon.slug, diff --git a/src/amo/api/index.js b/src/amo/api/index.js index c04c13b9c3b..bf4e3d39aa7 100644 --- a/src/amo/api/index.js +++ b/src/amo/api/index.js @@ -20,7 +20,7 @@ export type ApiReviewType = {| name: string, url: string, |}, - version?: {| + version: ?{| id: number, |}, |}; From a5c3cdb70955ee3be34802eb446275e1aa61910f Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 13:52:38 -0500 Subject: [PATCH 27/71] fixed missing ApiStateType --- src/core/components/AuthenticateButton/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/components/AuthenticateButton/index.js b/src/core/components/AuthenticateButton/index.js index cf5969892cc..9a3c09c6553 100644 --- a/src/core/components/AuthenticateButton/index.js +++ b/src/core/components/AuthenticateButton/index.js @@ -23,7 +23,7 @@ type HandleLogInFn = ( type HandleLogOutFn = ({| api: ApiStateType |}) => Promise; type AuthenticateButtonProps = {| - api: Object, + api: ApiStateType, className?: string, handleLogIn: HandleLogInFn, handleLogOut: HandleLogOutFn, From 213910f860d60d740ef15371b6689e70f8585b35 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 13:54:29 -0500 Subject: [PATCH 28/71] updated yarn.lock --- yarn.lock | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/yarn.lock b/yarn.lock index a0cd881bce3..cf6dc066fa6 100644 --- a/yarn.lock +++ b/yarn.lock @@ -297,15 +297,14 @@ babel-core@6.24.0, babel-core@^6.0.0, babel-core@^6.24.0: slash "^1.0.0" source-map "^0.5.0" -babel-eslint@7.2.0: - version "7.2.0" - resolved "https://registry.yarnpkg.com/babel-eslint/-/babel-eslint-7.2.0.tgz#8941514b9dead06f0df71b29d5d5b193a92ee0ae" +babel-eslint@7.2.1: + version "7.2.1" + resolved "https://registry.yarnpkg.com/babel-eslint/-/babel-eslint-7.2.1.tgz#079422eb73ba811e3ca0865ce87af29327f8c52f" dependencies: babel-code-frame "^6.22.0" babel-traverse "^6.23.1" babel-types "^6.23.0" babylon "^6.16.1" - lodash "^4.17.4" babel-generator@^6.18.0, babel-generator@^6.24.0: version "6.24.0" @@ -5219,9 +5218,9 @@ react-proxy@^1.1.7: lodash "^4.6.1" react-deep-force-update "^1.0.0" -react-redux-loading-bar@2.7.4: - version "2.7.4" - resolved "https://registry.yarnpkg.com/react-redux-loading-bar/-/react-redux-loading-bar-2.7.4.tgz#edf7bac3af910511320f7d3c7405450799de9958" +react-redux-loading-bar@2.8.0: + version "2.8.0" + resolved "https://registry.yarnpkg.com/react-redux-loading-bar/-/react-redux-loading-bar-2.8.0.tgz#041ef69f7474f2e77361bb7093e310b2c047088b" react-redux@4.4.6: version "4.4.6" From b6c2d6c1df35a5df30f5def2173c3ec8dfdbe99e Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 14:15:59 -0500 Subject: [PATCH 29/71] dev script with watch --- package.json | 4 +++- yarn.lock | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index c070c23aadb..0d86a344c0a 100644 --- a/package.json +++ b/package.json @@ -15,6 +15,7 @@ "dev:disco": "better-npm-run dev:disco", "eslint": "eslint .", "flow:check": "flow check", + "flow:dev": "watch flow src/ tests/", "stylelint": "stylelint --syntax scss **/*.scss", "lint": "npm run eslint && npm run stylelint", "servertest": "bin/config-check.js && ADDONS_FRONTEND_BUILD_ALL=1 npm run build && better-npm-run servertest && better-npm-run servertest:amo && better-npm-run servertest:disco && better-npm-run servertest:admin", @@ -119,7 +120,7 @@ } }, "test": { - "command": "npm run version-check && npm run unittest && npm run servertest && npm run eslint && npm run stylelint", + "command": "npm run version-check && npm run unittest && npm run servertest && npm run flow:check && npm run eslint && npm run stylelint", "env": { "NODE_PATH": "./:./src", "NODE_ENV": "test" @@ -288,6 +289,7 @@ "supertest-as-promised": "4.0.2", "svg-url-loader": "2.0.2", "tosource": "1.0.0", + "watch": "1.0.2", "webpack": "1.14.0", "webpack-dev-middleware": "1.10.1", "webpack-dev-server": "2.4.1", diff --git a/yarn.lock b/yarn.lock index cf6dc066fa6..fd515af5bc3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2360,6 +2360,12 @@ eventsource@0.1.6: dependencies: original ">=0.0.5" +exec-sh@^0.2.0: + version "0.2.0" + resolved "https://registry.yarnpkg.com/exec-sh/-/exec-sh-0.2.0.tgz#14f75de3f20d286ef933099b2ce50a90359cef10" + dependencies: + merge "^1.1.3" + execall@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/execall/-/execall-1.0.0.tgz#73d0904e395b3cab0658b08d09ec25307f29bb73" @@ -4061,6 +4067,10 @@ merge-descriptors@1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/merge-descriptors/-/merge-descriptors-1.0.1.tgz#b00aaa556dd8b44568150ec9d1b953f3f90cbb61" +merge@^1.1.3: + version "1.2.0" + resolved "https://registry.yarnpkg.com/merge/-/merge-1.2.0.tgz#7531e39d4949c281a66b8c5a6e0265e8b05894da" + methods@^1.1.1, methods@~1.1.2: version "1.1.2" resolved "https://registry.yarnpkg.com/methods/-/methods-1.1.2.tgz#5529a4d67654134edcc5266656835b0f851afcee" @@ -6592,6 +6602,13 @@ warning@^3.0.0: dependencies: loose-envify "^1.0.0" +watch@1.0.2: + version "1.0.2" + resolved "https://registry.yarnpkg.com/watch/-/watch-1.0.2.tgz#340a717bde765726fa0aa07d721e0147a551df0c" + dependencies: + exec-sh "^0.2.0" + minimist "^1.2.0" + watchpack@^0.2.1: version "0.2.9" resolved "https://registry.yarnpkg.com/watchpack/-/watchpack-0.2.9.tgz#62eaa4ab5e5ba35fdfc018275626e3c0f5e3fb0b" From d9d8775bd52d78b793df2a11105e35f3931bec8b Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 14:34:37 -0500 Subject: [PATCH 30/71] use chokidar for file watching --- package.json | 4 ++-- yarn.lock | 61 ++++++++++++++++++++++++++++++++++------------------ 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/package.json b/package.json index 0d86a344c0a..8d067e8dd1a 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,7 @@ "dev:disco": "better-npm-run dev:disco", "eslint": "eslint .", "flow:check": "flow check", - "flow:dev": "watch flow src/ tests/", + "flow:dev": "chokidar .flowconfig flow/ src/ tests/ -i flow/logs/flow.log -c 'flow status' --initial", "stylelint": "stylelint --syntax scss **/*.scss", "lint": "npm run eslint && npm run stylelint", "servertest": "bin/config-check.js && ADDONS_FRONTEND_BUILD_ALL=1 npm run build && better-npm-run servertest && better-npm-run servertest:amo && better-npm-run servertest:disco && better-npm-run servertest:admin", @@ -241,6 +241,7 @@ "chai": "3.5.0", "chalk": "1.1.3", "cheerio": "0.22.0", + "chokidar-cli": "1.2.0", "concurrently": "3.4.0", "cookie": "0.3.1", "css-loader": "0.27.3", @@ -289,7 +290,6 @@ "supertest-as-promised": "4.0.2", "svg-url-loader": "2.0.2", "tosource": "1.0.0", - "watch": "1.0.2", "webpack": "1.14.0", "webpack-dev-middleware": "1.10.1", "webpack-dev-server": "2.4.1", diff --git a/yarn.lock b/yarn.lock index fd515af5bc3..16f1ff4c8d3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -107,7 +107,7 @@ ansi-styles@~1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/ansi-styles/-/ansi-styles-1.0.0.tgz#cb102df1c56f5123eab8b67cd7b98027a0279178" -anymatch@^1.3.0: +anymatch@^1.1.0, anymatch@^1.3.0: version "1.3.0" resolved "https://registry.yarnpkg.com/anymatch/-/anymatch-1.3.0.tgz#a3e52fa39168c825ff57b0248126ce5a8ff95507" dependencies: @@ -149,6 +149,10 @@ array-equal@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/array-equal/-/array-equal-1.0.0.tgz#8c2a5ef2472fd9ea742b04c77a75093ba2757c93" +array-filter@~0.0.0: + version "0.0.1" + resolved "https://registry.yarnpkg.com/array-filter/-/array-filter-0.0.1.tgz#7da8cf2e26628ed732803581fd21f67cacd2eeec" + array-find-index@^1.0.1: version "1.0.2" resolved "https://registry.yarnpkg.com/array-find-index/-/array-find-index-1.0.2.tgz#df010aa1287e164bbda6f9723b0a96a1ec4187a1" @@ -157,6 +161,14 @@ array-flatten@1.1.1: version "1.1.1" resolved "https://registry.yarnpkg.com/array-flatten/-/array-flatten-1.1.1.tgz#9a5f699051b1e7073328f2a008968b64ea2955d2" +array-map@~0.0.0: + version "0.0.0" + resolved "https://registry.yarnpkg.com/array-map/-/array-map-0.0.0.tgz#88a2bab73d1cf7bcd5c1b118a003f66f665fa662" + +array-reduce@~0.0.0: + version "0.0.0" + resolved "https://registry.yarnpkg.com/array-reduce/-/array-reduce-0.0.0.tgz#173899d3ffd1c7d9383e4479525dbe278cab5f2b" + array-slice@^0.2.3: version "0.2.3" resolved "https://registry.yarnpkg.com/array-slice/-/array-slice-0.2.3.tgz#dd3cfb80ed7973a75117cdac69b0b99ec86186f5" @@ -1002,6 +1014,10 @@ block-stream@*: dependencies: inherits "~2.0.0" +bluebird@^2.9.24: + version "2.11.0" + resolved "https://registry.yarnpkg.com/bluebird/-/bluebird-2.11.0.tgz#534b9033c022c9579c56ba3b3e5a5caafbb650e1" + bluebird@^3.3.0, bluebird@^3.3.1: version "3.5.0" resolved "https://registry.yarnpkg.com/bluebird/-/bluebird-3.5.0.tgz#791420d7f551eea2897453a8a77653f96606d67c" @@ -1236,7 +1252,18 @@ cheerio@0.22.0: lodash.reject "^4.4.0" lodash.some "^4.4.0" -chokidar@^1.0.0, chokidar@^1.4.1, chokidar@^1.5.0, chokidar@^1.6.0: +chokidar-cli@1.2.0: + version "1.2.0" + resolved "https://registry.yarnpkg.com/chokidar-cli/-/chokidar-cli-1.2.0.tgz#8e7f58442273182018be1868e53c22af65a21948" + dependencies: + anymatch "^1.1.0" + bluebird "^2.9.24" + chokidar "^1.0.1" + lodash "^3.7.0" + shell-quote "^1.4.3" + yargs "^3.7.2" + +chokidar@^1.0.0, chokidar@^1.0.1, chokidar@^1.4.1, chokidar@^1.5.0, chokidar@^1.6.0: version "1.6.1" resolved "https://registry.yarnpkg.com/chokidar/-/chokidar-1.6.1.tgz#2f4447ab5e96e50fb3d789fd90d4c72e0e4c70c2" dependencies: @@ -2360,12 +2387,6 @@ eventsource@0.1.6: dependencies: original ">=0.0.5" -exec-sh@^0.2.0: - version "0.2.0" - resolved "https://registry.yarnpkg.com/exec-sh/-/exec-sh-0.2.0.tgz#14f75de3f20d286ef933099b2ce50a90359cef10" - dependencies: - merge "^1.1.3" - execall@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/execall/-/execall-1.0.0.tgz#73d0904e395b3cab0658b08d09ec25307f29bb73" @@ -3949,7 +3970,7 @@ lodash.uniq@^4.3.0: version "4.5.0" resolved "https://registry.yarnpkg.com/lodash.uniq/-/lodash.uniq-4.5.0.tgz#d0225373aeb652adc1bc82e4945339a842754773" -lodash@^3.8.0: +lodash@^3.7.0, lodash@^3.8.0: version "3.10.1" resolved "https://registry.yarnpkg.com/lodash/-/lodash-3.10.1.tgz#5bf45e8e49ba4189e17d482789dfd15bd140b7b6" @@ -4067,10 +4088,6 @@ merge-descriptors@1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/merge-descriptors/-/merge-descriptors-1.0.1.tgz#b00aaa556dd8b44568150ec9d1b953f3f90cbb61" -merge@^1.1.3: - version "1.2.0" - resolved "https://registry.yarnpkg.com/merge/-/merge-1.2.0.tgz#7531e39d4949c281a66b8c5a6e0265e8b05894da" - methods@^1.1.1, methods@~1.1.2: version "1.1.2" resolved "https://registry.yarnpkg.com/methods/-/methods-1.1.2.tgz#5529a4d67654134edcc5266656835b0f851afcee" @@ -5768,6 +5785,15 @@ shallowequal@^0.2.2: dependencies: lodash.keys "^3.1.2" +shell-quote@^1.4.3: + version "1.6.1" + resolved "https://registry.yarnpkg.com/shell-quote/-/shell-quote-1.6.1.tgz#f4781949cce402697127430ea3b3c5476f481767" + dependencies: + array-filter "~0.0.0" + array-map "~0.0.0" + array-reduce "~0.0.0" + jsonify "~0.0.0" + shelljs@0.7.7, shelljs@^0.7.5: version "0.7.7" resolved "https://registry.yarnpkg.com/shelljs/-/shelljs-0.7.7.tgz#b2f5c77ef97148f4b4f6e22682e10bba8667cff1" @@ -6602,13 +6628,6 @@ warning@^3.0.0: dependencies: loose-envify "^1.0.0" -watch@1.0.2: - version "1.0.2" - resolved "https://registry.yarnpkg.com/watch/-/watch-1.0.2.tgz#340a717bde765726fa0aa07d721e0147a551df0c" - dependencies: - exec-sh "^0.2.0" - minimist "^1.2.0" - watchpack@^0.2.1: version "0.2.9" resolved "https://registry.yarnpkg.com/watchpack/-/watchpack-0.2.9.tgz#62eaa4ab5e5ba35fdfc018275626e3c0f5e3fb0b" @@ -6866,7 +6885,7 @@ yargs@^1.2.6: version "1.3.3" resolved "https://registry.yarnpkg.com/yargs/-/yargs-1.3.3.tgz#054de8b61f22eefdb7207059eaef9d6b83fb931a" -yargs@^3.5.4, yargs@~3.10.0: +yargs@^3.5.4, yargs@^3.7.2, yargs@~3.10.0: version "3.10.0" resolved "https://registry.yarnpkg.com/yargs/-/yargs-3.10.0.tgz#f7ee7bd857dd7c1d2d38c0e74efbd681d1431fd1" dependencies: From 248356b82048d5dc14aa8f7ca6eeb1b40f6bfc26 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 14:51:03 -0500 Subject: [PATCH 31/71] docs for flow'ing --- README.md | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/README.md b/README.md index f42941b6abc..40e2c15fa45 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,8 @@ Generic scripts that don't need env vars. Use these for development: | npm run dev:amo | Starts the dev server and proxy (amo) | | npm run dev:amo:no-proxy| Starts the dev server without proxy (amo) | | npm run dev:disco | Starts the dev server (discovery pane) | +| npm run flow:check | Check for Flow errors and exit | +| npm run flow:dev | Continuously check for Flow errors | | npm run eslint | Lints the JS | | npm run stylelint | Lints the SCSS | | npm run lint | Runs all the JS + SCSS linters | @@ -84,6 +86,56 @@ or have `InfoDialog` in their behavior text. Any option after the double dash (`--`) gets sent to `mocha`. Check out [mocha's usage](https://mochajs.org/#usage) for ideas. +### Flow + +There is limited support for using [Flow](https://flowtype.org/) +to check for problems in the source code. + +Here is how to develop with constant feedback about Flow issues: + + npm run flow:dev + +Check out the [web-ext guide](https://github.com/mozilla/web-ext/blob/master/CONTRIBUTING.md#check-for-flow-errors) +for hints on how to solve common Flow errors. + +To add flow coverage to a source file, put a `/* @flow */` comment at the top. +The more source files you can opt into to Flow, the better. + +Here are some conventions we follow: + +* When a function like `getAllAddons` takes object arguments, call their + type object `GetAllAddonsParams`. For optional object arguments, call + their type object `GetAllAddonsOptions`. Example: + +````js +type GetAllAddonsParams = {| + author: string, +|}; + +type GetAllAddonsOptions = {| + version: string, +|}; + +function getAllAddons( + { author }: GetAllAddonsParams, + { version }: GetAllAddonsOptions = {} +) { + ... +} +```` + +* Always use [Exact object types](https://flowtype.org/en/docs/types/objects/#toc-exact-object-types) + using the pipe syntax (`{| key: ... |}`) when possible. Sometimes the + spread operator makes this difficult but you can use the + `Exact` workaround from `src/core/types/coreTypes` + if you have to. +* Try to avoid loose types like `Object` or `any` but feel free to use + them if you are falling down a rabbit hole. +* You can add a `$FLOW_FIXME` comment to skip a Flow check if you run + into a bug or if you hit something that's making you bang your head on + the keyboard. If it's something you think is unfixable then use + `$FLOW_IGNORE` instead. + ### Code coverage The `npm run unittest` command generates a report of how well the unit tests From 3cb62c057f559d77ce5cb94fc7e52094c22b0458 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 15:00:53 -0500 Subject: [PATCH 32/71] moar readme --- README.md | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 40e2c15fa45..0f0bf3455e8 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,7 @@ Check out the [web-ext guide](https://github.com/mozilla/web-ext/blob/master/CON for hints on how to solve common Flow errors. To add flow coverage to a source file, put a `/* @flow */` comment at the top. -The more source files you can opt into to Flow, the better. +The more source files you can opt into Flow, the better. Here are some conventions we follow: @@ -109,23 +109,23 @@ Here are some conventions we follow: ````js type GetAllAddonsParams = {| - author: string, + categoryId: number, |}; type GetAllAddonsOptions = {| - version: string, + hideDeleted: boolean, |}; function getAllAddons( - { author }: GetAllAddonsParams, - { version }: GetAllAddonsOptions = {} + { categoryId }: GetAllAddonsParams, + { hideDeleted = true }: GetAllAddonsOptions = {} ) { ... } ```` * Always use [Exact object types](https://flowtype.org/en/docs/types/objects/#toc-exact-object-types) - using the pipe syntax (`{| key: ... |}`) when possible. Sometimes the + via the pipe syntax (`{| key: ... |}`) when possible. Sometimes the spread operator makes this difficult but you can use the `Exact` workaround from `src/core/types/coreTypes` if you have to. @@ -135,6 +135,10 @@ function getAllAddons( into a bug or if you hit something that's making you bang your head on the keyboard. If it's something you think is unfixable then use `$FLOW_IGNORE` instead. +* The point of Flow is to help you *declare* the intention of your code and + help your team *understand* the implications of changes to interfaces and data + structures. If you feel like it is not doing this for you then ask someone + for guidance. ### Code coverage From 1411198d16e94ec538d8ac04f70c6838e33b87db Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 15:04:49 -0500 Subject: [PATCH 33/71] moar readme --- README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 0f0bf3455e8..6ab78a90654 100644 --- a/README.md +++ b/README.md @@ -101,8 +101,12 @@ for hints on how to solve common Flow errors. To add flow coverage to a source file, put a `/* @flow */` comment at the top. The more source files you can opt into Flow, the better. -Here are some conventions we follow: +Here is our Flow manifesto: +* The point of Flow is to help you declare the intention of your code and + help your team understand the implications of changes to interfaces and data + structures. It adds some verbosity and overhead but JavaScript hates you so + it is worth it. * When a function like `getAllAddons` takes object arguments, call their type object `GetAllAddonsParams`. For optional object arguments, call their type object `GetAllAddonsOptions`. Example: @@ -135,10 +139,6 @@ function getAllAddons( into a bug or if you hit something that's making you bang your head on the keyboard. If it's something you think is unfixable then use `$FLOW_IGNORE` instead. -* The point of Flow is to help you *declare* the intention of your code and - help your team *understand* the implications of changes to interfaces and data - structures. If you feel like it is not doing this for you then ask someone - for guidance. ### Code coverage From 77ce29c5d30297678ae7b61aa76c5ee26ad931fc Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 15:12:50 -0500 Subject: [PATCH 34/71] more manifesto! --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index 6ab78a90654..e2dabeac5f7 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,12 @@ Here is our Flow manifesto: help your team understand the implications of changes to interfaces and data structures. It adds some verbosity and overhead but JavaScript hates you so it is worth it. +* Try to avoid magic [Flow declarations](https://flowtype.org/en/docs/config/libs/) + for any *internal* code. Just declare a + [type alias](https://flowtype.org/en/docs/types/aliases/) next to the code + where it's used and export / import it like any other object. +* Never import a real JS object just to reference its type. Make a type alias + and import that instead. * When a function like `getAllAddons` takes object arguments, call their type object `GetAllAddonsParams`. For optional object arguments, call their type object `GetAllAddonsOptions`. Example: From e04027534f1e33a069d93db54f970d0b7302961d Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 27 Mar 2017 15:17:32 -0500 Subject: [PATCH 35/71] comment about Redux --- src/core/types/reduxTypes.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/types/reduxTypes.js b/src/core/types/reduxTypes.js index eebe48dde88..b67e2b7a014 100644 --- a/src/core/types/reduxTypes.js +++ b/src/core/types/reduxTypes.js @@ -1,3 +1,7 @@ /* @flow */ +// This defines some Redux interfaces that we want to depend on. +// It may be possible to use an official library for this. +// See: https://github.com/reactjs/react-redux/pull/389 +// and: https://github.com/reactjs/redux/pull/1887/files#diff-46d86d39c8da613247f843ee8ca43ebc export type DispatchFn = (action: Object) => void; From cf88101ad7ffcd5325ac544b759fd9d368d18cd1 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 29 Mar 2017 14:13:12 -0500 Subject: [PATCH 36/71] Clarified flow:dev --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e2dabeac5f7..c9834ff47d9 100644 --- a/README.md +++ b/README.md @@ -91,7 +91,7 @@ Any option after the double dash (`--`) gets sent to `mocha`. Check out There is limited support for using [Flow](https://flowtype.org/) to check for problems in the source code. -Here is how to develop with constant feedback about Flow issues: +To check for Flow issues during development while you edit files, run: npm run flow:dev From b698e5fc87bd9b68056190cb5611a55517a77aa5 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 29 Mar 2017 14:14:20 -0500 Subject: [PATCH 37/71] remove old artifacts directory --- artifacts/README.md | 1 - 1 file changed, 1 deletion(-) delete mode 100644 artifacts/README.md diff --git a/artifacts/README.md b/artifacts/README.md deleted file mode 100644 index 2b0352ebc6d..00000000000 --- a/artifacts/README.md +++ /dev/null @@ -1 +0,0 @@ -Ephemeral artifacts are saved to this directory. From 71980b0c8d804806a35ebd0f6e9d7c96bdec64e8 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 29 Mar 2017 14:18:55 -0500 Subject: [PATCH 38/71] run flow:check first --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 8d067e8dd1a..632d5b9ed68 100644 --- a/package.json +++ b/package.json @@ -120,7 +120,7 @@ } }, "test": { - "command": "npm run version-check && npm run unittest && npm run servertest && npm run flow:check && npm run eslint && npm run stylelint", + "command": "npm run version-check && npm run flow:check && npm run unittest && npm run servertest && npm run eslint && npm run stylelint", "env": { "NODE_PATH": "./:./src", "NODE_ENV": "test" From 1df442b03652b89eef4122e7f0e20a28ff878d95 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 29 Mar 2017 14:42:32 -0500 Subject: [PATCH 39/71] more docs --- README.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index c9834ff47d9..8f881591f03 100644 --- a/README.md +++ b/README.md @@ -95,8 +95,10 @@ To check for Flow issues during development while you edit files, run: npm run flow:dev -Check out the [web-ext guide](https://github.com/mozilla/web-ext/blob/master/CONTRIBUTING.md#check-for-flow-errors) -for hints on how to solve common Flow errors. +If you are new to working with Flow, here are some tips: +* Check out the [getting started](https://flow.org/en/docs/getting-started/) guide. +* Read through the [web-ext guide](https://github.com/mozilla/web-ext/blob/master/CONTRIBUTING.md#check-for-flow-errors) + for hints on how to solve common Flow errors. To add flow coverage to a source file, put a `/* @flow */` comment at the top. The more source files you can opt into Flow, the better. @@ -113,6 +115,9 @@ Here is our Flow manifesto: where it's used and export / import it like any other object. * Never import a real JS object just to reference its type. Make a type alias and import that instead. +* Never add more type annotations then you need to. Flow is really good at + inferring type information from standard JavaScript code. It will tell you + if you need to add an explicit annotation for something. * When a function like `getAllAddons` takes object arguments, call their type object `GetAllAddonsParams`. For optional object arguments, call their type object `GetAllAddonsOptions`. Example: From 6f23b6e36ad771fe9dea72986238a88d218ac3cf Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 29 Mar 2017 14:59:51 -0500 Subject: [PATCH 40/71] manifesto --- README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 8f881591f03..394deefd2df 100644 --- a/README.md +++ b/README.md @@ -105,10 +105,11 @@ The more source files you can opt into Flow, the better. Here is our Flow manifesto: -* The point of Flow is to help you declare the intention of your code and - help your team understand the implications of changes to interfaces and data +* The point of Flow is to help us declare the intention of our code and + help everyone understand the implications of changes to interfaces and data structures. It adds some verbosity and overhead but JavaScript hates you so - it is worth it. + it is worth it. Have you ever lost hair trying to track down why a variable + is undefined? *Of course you have*. * Try to avoid magic [Flow declarations](https://flowtype.org/en/docs/config/libs/) for any *internal* code. Just declare a [type alias](https://flowtype.org/en/docs/types/aliases/) next to the code From 4d5b6fc36270792bed7d7bf1e4c22c11bdfa49ab Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Thu, 30 Mar 2017 09:55:54 -0500 Subject: [PATCH 41/71] disable no-duplicate-imports --- .eslintrc | 4 +++- src/amo/components/RatingManager/index.js | 2 -- src/core/api/index.js | 2 -- src/core/components/AuthenticateButton/index.js | 2 -- 4 files changed, 3 insertions(+), 7 deletions(-) diff --git a/.eslintrc b/.eslintrc index f16632cbb2c..7b8b01a4223 100644 --- a/.eslintrc +++ b/.eslintrc @@ -55,7 +55,7 @@ }], // This ensures imports are at the top of the file. "import/imports-first": ["error"], - // This catches duplicate exports. + // This catches duplicate exports / imports. "import/no-duplicates": ["error"], // This ensures import statements never provide a file extension in the path. "import/extensions": ["error", "never"], @@ -72,6 +72,8 @@ "import/newline-after-import": ["error"], "jsx-a11y/no-static-element-interactions": "off", "no-console": "error", + // We use import/no-duplicates instead because it supports Flow types. + "no-duplicate-imports": "off", "no-plusplus": "off", "no-underscore-dangle": "off", "space-before-function-paren": ["error", "never"], diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 34c0804c098..88ca52991bb 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -21,7 +21,6 @@ import { import translate from 'core/i18n/translate'; import log from 'core/logger'; import DefaultRating from 'ui/components/Rating'; -/* eslint-disable no-duplicate-imports */ import type { ErrorHandlerType } from 'core/errorHandler'; import type { UserReviewType } from 'amo/actions/reviews'; import type { SubmitReviewParams } from 'amo/api'; @@ -30,7 +29,6 @@ import type { ApiStateType } from 'core/reducers/api'; import type { DispatchFn } from 'core/types/reduxTypes'; import type { AddonType, AddonTypeProp, AddonVersionType } from 'core/types/addonTypes'; -/* eslint-enable no-duplicate-imports */ import './styles.scss'; diff --git a/src/core/api/index.js b/src/core/api/index.js index b28e2da9af5..fe1ba7869e3 100644 --- a/src/core/api/index.js +++ b/src/core/api/index.js @@ -13,10 +13,8 @@ import { initialApiState } from 'core/reducers/api'; import { ADDON_TYPE_THEME } from 'core/constants'; import log from 'core/logger'; import { convertFiltersToQueryParams } from 'core/searchUtils'; -/* eslint-disable no-duplicate-imports */ import type { ErrorHandlerType } from 'core/errorHandler'; import type { ApiStateType } from 'core/reducers/api'; -/* eslint-enable no-duplicate-imports */ const API_BASE = `${config.get('apiHost')}${config.get('apiPath')}`; diff --git a/src/core/components/AuthenticateButton/index.js b/src/core/components/AuthenticateButton/index.js index 9a3c09c6553..75c72a6247e 100644 --- a/src/core/components/AuthenticateButton/index.js +++ b/src/core/components/AuthenticateButton/index.js @@ -10,11 +10,9 @@ import { logOutFromServer, startLoginUrl } from 'core/api'; import translate from 'core/i18n/translate'; import Button from 'ui/components/Button'; import Icon from 'ui/components/Icon'; -/* eslint-disable no-duplicate-imports */ import type { UrlFormatParams } from 'core/api'; import type { ApiStateType } from 'core/reducers/api'; import type { DispatchFn } from 'core/types/reduxTypes'; -/* eslint-enable no-duplicate-imports */ type HandleLogInFn = ( location: UrlFormatParams, options?: {| _window: typeof window |} From 052e097e33cabb080bcf3da06e4463e8eeb6d95b Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Thu, 30 Mar 2017 09:58:16 -0500 Subject: [PATCH 42/71] update yarn.lock after merge --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index 34d79480733..6827a202890 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1632,9 +1632,9 @@ css-color-names@0.0.4: version "0.0.4" resolved "https://registry.yarnpkg.com/css-color-names/-/css-color-names-0.0.4.tgz#808adc2e79cf84738069b646cb20ec27beb629e0" -css-loader@0.27.3: - version "0.27.3" - resolved "https://registry.yarnpkg.com/css-loader/-/css-loader-0.27.3.tgz#69ab6f47b69bfb1b5acee61bac2aab14302ff0dc" +css-loader@0.28.0: + version "0.28.0" + resolved "https://registry.yarnpkg.com/css-loader/-/css-loader-0.28.0.tgz#417cfa9789f8cde59a30ccbf3e4da7a806889bad" dependencies: babel-code-frame "^6.11.0" css-selector-tokenizer "^0.7.0" From eae1a6b152ccdb9b1d59115c42c9ea82140dbe3b Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Thu, 30 Mar 2017 15:48:53 -0500 Subject: [PATCH 43/71] update yarn.lock --- yarn.lock | 48 +++++++++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/yarn.lock b/yarn.lock index c4c544a6b8a..7142fc3574e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1159,7 +1159,7 @@ camelcase@^1.0.2: version "1.2.1" resolved "https://registry.yarnpkg.com/camelcase/-/camelcase-1.2.1.tgz#9bb5304d2e0b56698b2c758b08a3eaa9daa58a39" -camelcase@^2.0.0: +camelcase@^2.0.0, camelcase@^2.0.1: version "2.1.1" resolved "https://registry.yarnpkg.com/camelcase/-/camelcase-2.1.1.tgz#7c1d16d679a1bbe59ca02cacecfb011e201f5a1f" @@ -1314,7 +1314,7 @@ cliui@^2.1.0: right-align "^0.1.1" wordwrap "0.0.2" -cliui@^3.2.0: +cliui@^3.0.3, cliui@^3.2.0: version "3.2.0" resolved "https://registry.yarnpkg.com/cliui/-/cliui-3.2.0.tgz#120601537a916d29940f934da3b48d585a39213d" dependencies: @@ -4653,10 +4653,6 @@ path-is-inside@^1.0.1: version "1.0.2" resolved "https://registry.yarnpkg.com/path-is-inside/-/path-is-inside-1.0.2.tgz#365417dede44430d1c11af61027facf074bdfc53" -path-parse@^1.0.5: - version "1.0.5" - resolved "https://registry.yarnpkg.com/path-parse/-/path-parse-1.0.5.tgz#3c1adf871ea9cd6c9431b6ea2bd74a0ff055c4c1" - path-to-regexp@0.1.7: version "0.1.7" resolved "https://registry.yarnpkg.com/path-to-regexp/-/path-to-regexp-0.1.7.tgz#df604178005f522f15eb4490e7247a1bfaa67f8c" @@ -5651,16 +5647,10 @@ resolve-from@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/resolve-from/-/resolve-from-2.0.0.tgz#9480ab20e94ffa1d9e80a804c7ea147611966b57" -resolve@1.1.x: +resolve@1.1.x, resolve@^1.1.6: version "1.1.7" resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.1.7.tgz#203114d82ad2c5ed9e8e0411b3932875e889e97b" -resolve@^1.1.6: - version "1.3.2" - resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.3.2.tgz#1f0442c9e0cbb8136e87b9305f932f46c7f28235" - dependencies: - path-parse "^1.0.5" - restore-cursor@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/restore-cursor/-/restore-cursor-1.0.1.tgz#34661f46886327fed2991479152252df92daa541" @@ -6856,6 +6846,10 @@ window-size@0.1.0: version "0.1.0" resolved "https://registry.yarnpkg.com/window-size/-/window-size-0.1.0.tgz#5438cd2ea93b202efa3a19fe8887aee7c94f9c9d" +window-size@^0.1.4: + version "0.1.4" + resolved "https://registry.yarnpkg.com/window-size/-/window-size-0.1.4.tgz#f8e1aa1ee5a53ec5bf151ffa09742a6ad7697876" + window-size@^0.2.0: version "0.2.0" resolved "https://registry.yarnpkg.com/window-size/-/window-size-0.2.0.tgz#b4315bb4214a3d7058ebeee892e13fa24d98b075" @@ -6920,7 +6914,7 @@ xmlhttprequest-ssl@1.5.3: version "4.0.1" resolved "https://registry.yarnpkg.com/xtend/-/xtend-4.0.1.tgz#a5c6d532be656e23db820efb943a1f04998d63af" -y18n@^3.2.1: +y18n@^3.2.0, y18n@^3.2.1: version "3.2.1" resolved "https://registry.yarnpkg.com/y18n/-/y18n-3.2.1.tgz#6d15fba884c08679c0d77e88e7759e811e07fa41" @@ -6945,14 +6939,17 @@ yargs@^1.2.6: version "1.3.3" resolved "https://registry.yarnpkg.com/yargs/-/yargs-1.3.3.tgz#054de8b61f22eefdb7207059eaef9d6b83fb931a" -yargs@^3.5.4, yargs@^3.7.2, yargs@~3.10.0: - version "3.10.0" - resolved "https://registry.yarnpkg.com/yargs/-/yargs-3.10.0.tgz#f7ee7bd857dd7c1d2d38c0e74efbd681d1431fd1" +yargs@^3.5.4, yargs@^3.7.2: + version "3.32.0" + resolved "https://registry.yarnpkg.com/yargs/-/yargs-3.32.0.tgz#03088e9ebf9e756b69751611d2a5ef591482c995" dependencies: - camelcase "^1.0.2" - cliui "^2.1.0" - decamelize "^1.0.0" - window-size "0.1.0" + camelcase "^2.0.1" + cliui "^3.0.3" + decamelize "^1.1.1" + os-locale "^1.4.0" + string-width "^1.0.1" + window-size "^0.1.4" + y18n "^3.2.0" yargs@^4.7.1: version "4.8.1" @@ -6991,6 +6988,15 @@ yargs@^6.0.0: y18n "^3.2.1" yargs-parser "^4.2.0" +yargs@~3.10.0: + version "3.10.0" + resolved "https://registry.yarnpkg.com/yargs/-/yargs-3.10.0.tgz#f7ee7bd857dd7c1d2d38c0e74efbd681d1431fd1" + dependencies: + camelcase "^1.0.2" + cliui "^2.1.0" + decamelize "^1.0.0" + window-size "0.1.0" + yeast@0.1.2: version "0.1.2" resolved "https://registry.yarnpkg.com/yeast/-/yeast-0.1.2.tgz#008e06d8094320c372dbc2f8ed76a0ca6c8ac419" From 732cebdb6a9b2987b505687be588a4bfc94667ee Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 5 Apr 2017 14:46:36 -0500 Subject: [PATCH 44/71] No more separation of params vs options because I forgot that this is a web-ext convention related to yargs. --- README.md | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 3adb2cb1510..b9865f0707b 100644 --- a/README.md +++ b/README.md @@ -119,23 +119,15 @@ Here is our Flow manifesto: * Never add more type annotations then you need to. Flow is really good at inferring type information from standard JavaScript code. It will tell you if you need to add an explicit annotation for something. -* When a function like `getAllAddons` takes object arguments, call their - type object `GetAllAddonsParams`. For optional object arguments, call - their type object `GetAllAddonsOptions`. Example: +* When a function like `getAllAddons` takes object arguments, call its + type object `GetAllAddonsParams`. Example: ````js type GetAllAddonsParams = {| categoryId: number, |}; -type GetAllAddonsOptions = {| - hideDeleted: boolean, -|}; - -function getAllAddons( - { categoryId }: GetAllAddonsParams, - { hideDeleted = true }: GetAllAddonsOptions = {} -) { +function getAllAddons({ categoryId }: GetAllAddonsParams = {}) { ... } ```` From c5d9b19ed6ed365ac45a4d4746b8b317aed0939d Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 10:42:17 -0500 Subject: [PATCH 45/71] sync yarn.lock and alphabetize babel plugins --- .babelrc | 4 ++-- yarn.lock | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.babelrc b/.babelrc index 4a6d9d4dd7f..f67f55cdfe3 100644 --- a/.babelrc +++ b/.babelrc @@ -5,9 +5,9 @@ "react" ], "plugins": [ - "transform-flow-strip-types", "babel-plugin-dedent", "transform-class-properties", - "transform-es2015-modules-commonjs" + "transform-es2015-modules-commonjs", + "transform-flow-strip-types" ] } diff --git a/yarn.lock b/yarn.lock index bce4ec47bc4..35d138c8670 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1117,9 +1117,9 @@ bundle-loader@0.5.5: dependencies: loader-utils "^1.0.2" -bunyan@1.8.9: - version "1.8.9" - resolved "https://registry.yarnpkg.com/bunyan/-/bunyan-1.8.9.tgz#2c7c9d422ea64ee2465d52b4decd72de0657401a" +bunyan@1.8.10: + version "1.8.10" + resolved "https://registry.yarnpkg.com/bunyan/-/bunyan-1.8.10.tgz#201fedd26c7080b632f416072f53a90b9a52981c" optionalDependencies: dtrace-provider "~0.8" moment "^2.10.6" @@ -1817,13 +1817,13 @@ debug@2.3.3: dependencies: ms "0.7.2" -debug@2.6.1, debug@^2.1.1, debug@^2.2.0: +debug@2.6.1, debug@^2.1.1, debug@^2.2.0, debug@^2.6.0: version "2.6.1" resolved "https://registry.yarnpkg.com/debug/-/debug-2.6.1.tgz#79855090ba2c4e3115cc7d8769491d58f0491351" dependencies: ms "0.7.2" -debug@2.6.3, debug@^2.6.0: +debug@2.6.3: version "2.6.3" resolved "https://registry.yarnpkg.com/debug/-/debug-2.6.3.tgz#0f7eb8c30965ec08c72accfa0130c8b79984141d" dependencies: From cd8712b17fe7838d43c9783be770e8f6e3abeba2 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 10:44:15 -0500 Subject: [PATCH 46/71] better import/export comment --- .eslintrc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.eslintrc b/.eslintrc index 7b8b01a4223..ba9f7722599 100644 --- a/.eslintrc +++ b/.eslintrc @@ -55,7 +55,7 @@ }], // This ensures imports are at the top of the file. "import/imports-first": ["error"], - // This catches duplicate exports / imports. + // This reports when you accidentally import / export an object twice. "import/no-duplicates": ["error"], // This ensures import statements never provide a file extension in the path. "import/extensions": ["error", "never"], From ec2425476c16eb5058beac19a5117e1775285f0e Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 10:45:23 -0500 Subject: [PATCH 47/71] fixed flow comment --- .flowconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.flowconfig b/.flowconfig index d3b368ac22a..af8ef0e1857 100644 --- a/.flowconfig +++ b/.flowconfig @@ -1,5 +1,5 @@ [ignore] -# Ignore minified addons-frontend code. +# Ignore built/minified addons-frontend code. /dist/.* # These modules opt into Flow but we don't need to check them. From 892fe905bf9fa8a0aa6639f392b00cb5961f6a75 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 10:47:23 -0500 Subject: [PATCH 48/71] todo: remove dedent declarations later --- .flowconfig | 2 ++ flow/libs/dedent.js.flow | 3 +++ 2 files changed, 5 insertions(+) diff --git a/.flowconfig b/.flowconfig index af8ef0e1857..ddea1631f59 100644 --- a/.flowconfig +++ b/.flowconfig @@ -10,6 +10,8 @@ [include] [libs] +# TODO: this can go away after +# https://github.com/mozilla/addons-frontend/issues/2092 ./flow/libs/dedent.js.flow [options] diff --git a/flow/libs/dedent.js.flow b/flow/libs/dedent.js.flow index 8738e089e09..44671e9eeac 100644 --- a/flow/libs/dedent.js.flow +++ b/flow/libs/dedent.js.flow @@ -1,3 +1,6 @@ +// TODO: this can go away after +// https://github.com/mozilla/addons-frontend/issues/2092 + // I'm not sure exactly what this should be. I was using this issue as a guide. // https://github.com/facebook/flow/issues/2616#issuecomment-289257544 declare function dedent(params: Array<*>): string; From cf66983bd177442321f1cf343c830143a9f1c2d1 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 10:53:55 -0500 Subject: [PATCH 49/71] improved sass comment --- .flowconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.flowconfig b/.flowconfig index ddea1631f59..6da803b7eb5 100644 --- a/.flowconfig +++ b/.flowconfig @@ -15,7 +15,8 @@ ./flow/libs/dedent.js.flow [options] -# This maps all sass imports to a dummy flow file to suppress import errors. +# This maps all Sass/SCSS imports to a dummy Flow file to suppress import +# errors. It's not necessary for Flow to analyze Sass/SCSS files. module.name_mapper.extension='scss' -> '/flow/flowStub.js.flow' module.system=node module.system.node.resolve_dirname=node_modules From 48cf36fc92bfa8f012d07f2e06096e03f49ecf97 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:07:22 -0500 Subject: [PATCH 50/71] more concise intro --- README.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index b9865f0707b..b7283639cd2 100644 --- a/README.md +++ b/README.md @@ -105,11 +105,10 @@ The more source files you can opt into Flow, the better. Here is our Flow manifesto: -* The point of Flow is to help us declare the intention of our code and - help everyone understand the implications of changes to interfaces and data - structures. It adds some verbosity and overhead but JavaScript hates you so - it is worth it. Have you ever lost hair trying to track down why a variable - is undefined? *Of course you have*. +* We use Flow to **declare the intention of our code** and help others + refactor it with confidence. + Flow also makes it easier to catch mistakes before spending hours in a debugger + trying to find out what happened. * Try to avoid magic [Flow declarations](https://flowtype.org/en/docs/config/libs/) for any *internal* code. Just declare a [type alias](https://flowtype.org/en/docs/types/aliases/) next to the code From 898a45e4337a8ab75abe31ca7d24dd61c926a8da Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:11:05 -0500 Subject: [PATCH 51/71] no spaces between slashes --- .eslintrc | 2 +- README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.eslintrc b/.eslintrc index ba9f7722599..35af0bb1855 100644 --- a/.eslintrc +++ b/.eslintrc @@ -55,7 +55,7 @@ }], // This ensures imports are at the top of the file. "import/imports-first": ["error"], - // This reports when you accidentally import / export an object twice. + // This reports when you accidentally import/export an object twice. "import/no-duplicates": ["error"], // This ensures import statements never provide a file extension in the path. "import/extensions": ["error", "never"], diff --git a/README.md b/README.md index b7283639cd2..f4142eec1b1 100644 --- a/README.md +++ b/README.md @@ -112,7 +112,7 @@ Here is our Flow manifesto: * Try to avoid magic [Flow declarations](https://flowtype.org/en/docs/config/libs/) for any *internal* code. Just declare a [type alias](https://flowtype.org/en/docs/types/aliases/) next to the code - where it's used and export / import it like any other object. + where it's used and export/import it like any other object. * Never import a real JS object just to reference its type. Make a type alias and import that instead. * Never add more type annotations then you need to. Flow is really good at From aa46b391ca41121be7066986240232f8a4a9dd0a Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:23:39 -0500 Subject: [PATCH 52/71] fixed avoiding magic section --- README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index f4142eec1b1..721a1e0cbf9 100644 --- a/README.md +++ b/README.md @@ -109,10 +109,11 @@ Here is our Flow manifesto: refactor it with confidence. Flow also makes it easier to catch mistakes before spending hours in a debugger trying to find out what happened. -* Try to avoid magic [Flow declarations](https://flowtype.org/en/docs/config/libs/) +* Avoid magic [Flow declarations](https://flowtype.org/en/docs/config/libs/) for any *internal* code. Just declare a [type alias](https://flowtype.org/en/docs/types/aliases/) next to the code - where it's used and export/import it like any other object. + where it's used and + [export/import](https://flow.org/en/docs/types/modules/) it like any other object. * Never import a real JS object just to reference its type. Make a type alias and import that instead. * Never add more type annotations then you need to. Flow is really good at From 9ab7b383af81d043c16e08264b066cbc9a4f3ce3 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:25:44 -0500 Subject: [PATCH 53/71] fixed flow docs --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 721a1e0cbf9..5ae8c65ce05 100644 --- a/README.md +++ b/README.md @@ -116,9 +116,9 @@ Here is our Flow manifesto: [export/import](https://flow.org/en/docs/types/modules/) it like any other object. * Never import a real JS object just to reference its type. Make a type alias and import that instead. -* Never add more type annotations then you need to. Flow is really good at - inferring type information from standard JavaScript code. It will tell you - if you need to add an explicit annotation for something. +* Never add more type annotations than you need. Flow is really good at + inferring types from standard JavaScript code; it will tell you + when you need to add explicit annotations. * When a function like `getAllAddons` takes object arguments, call its type object `GetAllAddonsParams`. Example: From 68a4d8a388d3f6cd8c9c0924c7789c63da938e9e Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:26:14 -0500 Subject: [PATCH 54/71] fix JS --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5ae8c65ce05..457a856b495 100644 --- a/README.md +++ b/README.md @@ -117,7 +117,7 @@ Here is our Flow manifesto: * Never import a real JS object just to reference its type. Make a type alias and import that instead. * Never add more type annotations than you need. Flow is really good at - inferring types from standard JavaScript code; it will tell you + inferring types from standard JS code; it will tell you when you need to add explicit annotations. * When a function like `getAllAddons` takes object arguments, call its type object `GetAllAddonsParams`. Example: From cec7c56de7989c774810b69cc7effa4b45d023fb Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:27:30 -0500 Subject: [PATCH 55/71] fix docs --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 457a856b495..3548e437bfc 100644 --- a/README.md +++ b/README.md @@ -132,9 +132,9 @@ function getAllAddons({ categoryId }: GetAllAddonsParams = {}) { } ```` -* Always use [Exact object types](https://flowtype.org/en/docs/types/objects/#toc-exact-object-types) +* Use [Exact object types](https://flowtype.org/en/docs/types/objects/#toc-exact-object-types) via the pipe syntax (`{| key: ... |}`) when possible. Sometimes the - spread operator makes this difficult but you can use the + spread operator makes this difficult so you can use the `Exact` workaround from `src/core/types/coreTypes` if you have to. * Try to avoid loose types like `Object` or `any` but feel free to use From 69ff7896166980b43a9afb0609fc07222d611060 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:36:18 -0500 Subject: [PATCH 56/71] better Exact workaround explanation --- README.md | 3 ++- src/core/types/coreTypes.js | 20 +++++++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 3548e437bfc..4eda5c04e42 100644 --- a/README.md +++ b/README.md @@ -135,7 +135,8 @@ function getAllAddons({ categoryId }: GetAllAddonsParams = {}) { * Use [Exact object types](https://flowtype.org/en/docs/types/objects/#toc-exact-object-types) via the pipe syntax (`{| key: ... |}`) when possible. Sometimes the spread operator makes this difficult so you can use the - `Exact` workaround from `src/core/types/coreTypes` + `Exact` workaround from + [`src/core/types/coreTypes`](https://github.com/mozilla/addons-frontend/blob/master/src/core/types/coreTypes.js) if you have to. * Try to avoid loose types like `Object` or `any` but feel free to use them if you are falling down a rabbit hole. diff --git a/src/core/types/coreTypes.js b/src/core/types/coreTypes.js index 1c63c17ab79..32c399c7ab8 100644 --- a/src/core/types/coreTypes.js +++ b/src/core/types/coreTypes.js @@ -1,6 +1,24 @@ /* @flow */ /* global $Shape */ -// This is a workaround for exact object types that support object spreads. +// This is a workaround for exact object types when you can't use the pipe +// syntax: https://flowtype.org/en/docs/types/objects/#toc-exact-object-types +// +// At the time of this writing, you may need to use this workaround if you +// want to use the spread operator to merge objects together. See: // https://github.com/facebook/flow/issues/2405#issuecomment-274073091 +// +// Usage: +// +// Let's say you have a loosely defined parameter type for a function like this: +// +// type GetAllAddonsParams = { +// categoryId: number, +// }; +// +// You can make sure this function accepts *only* these parameters like this: +// +// function getAllAddons({ categoryId }: Exact = {}) { +// ... +// } export type Exact = T & $Shape; From 7085a4d97f005935a29f4c873caf2513e1df4446 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:40:05 -0500 Subject: [PATCH 57/71] made 'rabbit hole' more explicit --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 4eda5c04e42..70ab73e84af 100644 --- a/README.md +++ b/README.md @@ -139,7 +139,8 @@ function getAllAddons({ categoryId }: GetAllAddonsParams = {}) { [`src/core/types/coreTypes`](https://github.com/mozilla/addons-frontend/blob/master/src/core/types/coreTypes.js) if you have to. * Try to avoid loose types like `Object` or `any` but feel free to use - them if you are falling down a rabbit hole. + them if you are spending too much time declaring types that depend on other + types that depend on other types, and so on. * You can add a `$FLOW_FIXME` comment to skip a Flow check if you run into a bug or if you hit something that's making you bang your head on the keyboard. If it's something you think is unfixable then use From d643a3c4bdf798de11c0989d990dd18ac34b0c57 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:45:33 -0500 Subject: [PATCH 58/71] flow docs --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 70ab73e84af..30376a30b17 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,8 @@ function getAllAddons({ categoryId }: GetAllAddonsParams = {}) { * You can add a `$FLOW_FIXME` comment to skip a Flow check if you run into a bug or if you hit something that's making you bang your head on the keyboard. If it's something you think is unfixable then use - `$FLOW_IGNORE` instead. + `$FLOW_IGNORE` instead. Please explain your rationale in the comment and link + to a github issue if possible. ### Code coverage From 2bc4a662fb5f920826ff5017bd3682b8aa0c62f8 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:50:00 -0500 Subject: [PATCH 59/71] reformat imports --- src/amo/components/RatingManager/index.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 88ca52991bb..e69eca1e7a2 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -27,8 +27,11 @@ import type { SubmitReviewParams } from 'amo/api'; import type { UrlFormatParams } from 'core/api'; import type { ApiStateType } from 'core/reducers/api'; import type { DispatchFn } from 'core/types/reduxTypes'; -import type { AddonType, AddonTypeProp, AddonVersionType } - from 'core/types/addonTypes'; +import type { + AddonType, + AddonTypeProp, + AddonVersionType +} from 'core/types/addonTypes'; import './styles.scss'; From 649458509a801a7ef56e9dff13f37f24eaf570aa Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:53:12 -0500 Subject: [PATCH 60/71] Use func not fn abbreviation --- src/amo/components/RatingManager/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index e69eca1e7a2..04c639cba88 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -35,7 +35,7 @@ import type { import './styles.scss'; -type LoadSavedReviewFn = ({| +type LoadSavedReviewFunc = ({| userId: number, addonId: number, |}) => Promise<*>; @@ -50,7 +50,7 @@ type RatingManagerProps = {| errorHandler: ErrorHandlerType, apiState: ApiStateType, i18n: Object, - loadSavedReview: LoadSavedReviewFn, + loadSavedReview: LoadSavedReviewFunc, location: UrlFormatParams, submitReview: SubmitReviewFn, userId: number, @@ -226,7 +226,7 @@ export const mapStateToProps = ( }; type DispatchMappedProps = {| - loadSavedReview: LoadSavedReviewFn, + loadSavedReview: LoadSavedReviewFunc, submitReview: SubmitReviewFn, |} From 830d23df7167fbe7d65c771ae4b8d08817bc1ad2 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 11:53:41 -0500 Subject: [PATCH 61/71] use 'any' not star --- src/amo/components/RatingManager/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 04c639cba88..ccdf16b9091 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -38,7 +38,7 @@ import './styles.scss'; type LoadSavedReviewFunc = ({| userId: number, addonId: number, -|}) => Promise<*>; +|}) => Promise; type SubmitReviewFn = (SubmitReviewParams) => Promise; From ff6047392c08d9acba89df60e871b598a1c9466e Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 12:14:15 -0500 Subject: [PATCH 62/71] Fn -> Func --- src/amo/components/RatingManager/index.js | 12 ++++++------ src/core/components/AuthenticateButton/index.js | 16 ++++++++-------- src/core/types/reduxTypes.js | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index ccdf16b9091..cf3b64e2559 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -26,7 +26,7 @@ import type { UserReviewType } from 'amo/actions/reviews'; import type { SubmitReviewParams } from 'amo/api'; import type { UrlFormatParams } from 'core/api'; import type { ApiStateType } from 'core/reducers/api'; -import type { DispatchFn } from 'core/types/reduxTypes'; +import type { DispatchFunc } from 'core/types/reduxTypes'; import type { AddonType, AddonTypeProp, @@ -40,19 +40,19 @@ type LoadSavedReviewFunc = ({| addonId: number, |}) => Promise; -type SubmitReviewFn = (SubmitReviewParams) => Promise; +type SubmitReviewFunc = (SubmitReviewParams) => Promise; type RatingManagerProps = {| AddonReview: typeof DefaultAddonReview, AuthenticateButton: typeof DefaultAuthenticateButton, Rating: typeof DefaultRating, addon: AddonType, - errorHandler: ErrorHandlerType, apiState: ApiStateType, + errorHandler: ErrorHandlerType, i18n: Object, loadSavedReview: LoadSavedReviewFunc, location: UrlFormatParams, - submitReview: SubmitReviewFn, + submitReview: SubmitReviewFunc, userId: number, userReview: UserReviewType, version: AddonVersionType, @@ -227,11 +227,11 @@ export const mapStateToProps = ( type DispatchMappedProps = {| loadSavedReview: LoadSavedReviewFunc, - submitReview: SubmitReviewFn, + submitReview: SubmitReviewFunc, |} export const mapDispatchToProps = ( - dispatch: DispatchFn + dispatch: DispatchFunc ): DispatchMappedProps => ({ loadSavedReview({ userId, addonId }) { diff --git a/src/core/components/AuthenticateButton/index.js b/src/core/components/AuthenticateButton/index.js index 75c72a6247e..4d640c578c7 100644 --- a/src/core/components/AuthenticateButton/index.js +++ b/src/core/components/AuthenticateButton/index.js @@ -12,19 +12,19 @@ import Button from 'ui/components/Button'; import Icon from 'ui/components/Icon'; import type { UrlFormatParams } from 'core/api'; import type { ApiStateType } from 'core/reducers/api'; -import type { DispatchFn } from 'core/types/reduxTypes'; +import type { DispatchFunc } from 'core/types/reduxTypes'; -type HandleLogInFn = ( +type HandleLogInFunc = ( location: UrlFormatParams, options?: {| _window: typeof window |} ) => void; -type HandleLogOutFn = ({| api: ApiStateType |}) => Promise; +type HandleLogOutFunc = ({| api: ApiStateType |}) => Promise; type AuthenticateButtonProps = {| api: ApiStateType, className?: string, - handleLogIn: HandleLogInFn, - handleLogOut: HandleLogOutFn, + handleLogIn: HandleLogInFunc, + handleLogOut: HandleLogOutFunc, i18n: Object, isAuthenticated: boolean, location: UrlFormatParams, @@ -72,7 +72,7 @@ export class AuthenticateButtonBase extends React.Component { type StateMappedProps = {| api: ApiStateType, isAuthenticated: boolean, - handleLogIn: HandleLogInFn, + handleLogIn: HandleLogInFunc, |}; export const mapStateToProps = ( @@ -87,11 +87,11 @@ export const mapStateToProps = ( }); type DispatchMappedProps = {| - handleLogOut: HandleLogOutFn, + handleLogOut: HandleLogOutFunc, |}; export const mapDispatchToProps = ( - dispatch: DispatchFn + dispatch: DispatchFunc ): DispatchMappedProps => ({ handleLogOut({ api }) { return logOutFromServer({ api }) diff --git a/src/core/types/reduxTypes.js b/src/core/types/reduxTypes.js index b67e2b7a014..d69d2e93eeb 100644 --- a/src/core/types/reduxTypes.js +++ b/src/core/types/reduxTypes.js @@ -4,4 +4,4 @@ // It may be possible to use an official library for this. // See: https://github.com/reactjs/react-redux/pull/389 // and: https://github.com/reactjs/redux/pull/1887/files#diff-46d86d39c8da613247f843ee8ca43ebc -export type DispatchFn = (action: Object) => void; +export type DispatchFunc = (action: Object) => void; From 8ce880fb728e2ebece5ffd9b8145f044c3f16dab Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 17:15:07 -0500 Subject: [PATCH 63/71] remove irrelevant typeof declarations --- src/amo/actions/reviews.js | 4 ++-- src/amo/components/RatingManager/index.js | 8 ++------ src/core/actions/index.js | 14 +++++++------- src/core/types/addonTypes.js | 17 +---------------- 4 files changed, 12 insertions(+), 31 deletions(-) diff --git a/src/amo/actions/reviews.js b/src/amo/actions/reviews.js index 2e8bce879db..bd76e304c5b 100644 --- a/src/amo/actions/reviews.js +++ b/src/amo/actions/reviews.js @@ -36,7 +36,7 @@ export function denormalizeReview(review: ApiReviewType): UserReviewType { } export type SetReviewAction = {| - type: typeof SET_REVIEW, + type: string, payload: UserReviewType, |}; @@ -57,7 +57,7 @@ export const setDenormalizedReview = ( }; export type SetAddonReviewsAction = {| - type: typeof SET_ADDON_REVIEWS, + type: string, payload: {| addonSlug: string, reviews: Array, diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index cf3b64e2559..67057ea43ca 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -27,11 +27,7 @@ import type { SubmitReviewParams } from 'amo/api'; import type { UrlFormatParams } from 'core/api'; import type { ApiStateType } from 'core/reducers/api'; import type { DispatchFunc } from 'core/types/reduxTypes'; -import type { - AddonType, - AddonTypeProp, - AddonVersionType -} from 'core/types/addonTypes'; +import type { AddonType, AddonVersionType } from 'core/types/addonTypes'; import './styles.scss'; @@ -113,7 +109,7 @@ export class RatingManagerBase extends React.Component { } getLogInPrompt( - { addonType }: {| addonType: AddonTypeProp |}, + { addonType }: {| addonType: string |}, { validAddonTypes = defaultValidAddonTypes, }: {| diff --git a/src/core/actions/index.js b/src/core/actions/index.js index 5b8fae179ca..16a41e43451 100644 --- a/src/core/actions/index.js +++ b/src/core/actions/index.js @@ -10,7 +10,7 @@ import { } from 'core/constants'; export type SetAuthTokenAction = {| - type: typeof SET_AUTH_TOKEN, + type: string, payload: {| token: string |}, |}; @@ -25,7 +25,7 @@ export function setAuthToken(token: string): SetAuthTokenAction { } export type LogOutUserAction = {| - type: typeof LOG_OUT_USER, + type: string, |}; export function logOutUser(): LogOutUserAction { @@ -33,7 +33,7 @@ export function logOutUser(): LogOutUserAction { } export type SetClientAppAction = {| - type: typeof SET_CLIENT_APP, + type: string, payload: {| clientApp: string |}, |}; @@ -48,7 +48,7 @@ export function setClientApp(clientApp: string): SetClientAppAction { } export type SetLangAction = {| - type: typeof SET_LANG, + type: string, payload: {| lang: string |}, |}; @@ -60,7 +60,7 @@ export function setLang(lang: string): SetLangAction { } export type SetUserAgentAction = {| - type: typeof SET_USER_AGENT, + type: string, payload: {| userAgent: string |}, |}; @@ -72,7 +72,7 @@ export function setUserAgent(userAgent: string): SetUserAgentAction { } export type LoadEntitiesAction = {| - type: typeof ENTITIES_LOADED, + type: string, payload: {| entities: Array |}, |}; @@ -84,7 +84,7 @@ export function loadEntities(entities: Array): LoadEntitiesAction { } export type SetCurrentUserAction = {| - type: typeof SET_CURRENT_USER, + type: string, payload: {| username: string |}, |}; diff --git a/src/core/types/addonTypes.js b/src/core/types/addonTypes.js index bcddbeb3d4b..607efd3feb5 100644 --- a/src/core/types/addonTypes.js +++ b/src/core/types/addonTypes.js @@ -1,12 +1,4 @@ /* @flow */ -import { - ADDON_TYPE_DICT, - ADDON_TYPE_EXTENSION, - ADDON_TYPE_LANG, - ADDON_TYPE_SEARCH, - ADDON_TYPE_THEME, -} from 'core/constants'; - export type AddonVersionType = {| id: number, channel: string, @@ -23,13 +15,6 @@ export type AddonAuthorType = {| url: string, |}; -export type AddonTypeProp = - typeof ADDON_TYPE_DICT | - typeof ADDON_TYPE_LANG | - typeof ADDON_TYPE_SEARCH | - typeof ADDON_TYPE_THEME | - typeof ADDON_TYPE_EXTENSION; - export type AddonType = {| id: number, authors: Array, @@ -76,7 +61,7 @@ export type AddonType = {| support_url: string, tags: Array, theme_data: Object, - type: AddonTypeProp, + type: string, url: string, weekly_downloads: number, |}; From 8d7f1d8ced99599cb68a614c50a78276d112005d Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 17:41:11 -0500 Subject: [PATCH 64/71] More thorough Exact explanation --- README.md | 9 ++++++--- src/core/types/coreTypes.js | 10 ++++++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 30376a30b17..8bea9da0dcf 100644 --- a/README.md +++ b/README.md @@ -134,10 +134,13 @@ function getAllAddons({ categoryId }: GetAllAddonsParams = {}) { * Use [Exact object types](https://flowtype.org/en/docs/types/objects/#toc-exact-object-types) via the pipe syntax (`{| key: ... |}`) when possible. Sometimes the - spread operator makes this difficult so you can use the - `Exact` workaround from + spread operator triggers an error like + 'Inexact type is incompatible with exact type' but that's a + [bug](https://github.com/facebook/flow/issues/2405). + You can use the `Exact` workaround from [`src/core/types/coreTypes`](https://github.com/mozilla/addons-frontend/blob/master/src/core/types/coreTypes.js) - if you have to. + if you have to. This is meant as a working replacement for + [$Exact](https://flow.org/en/docs/types/utilities/#toc-exact). * Try to avoid loose types like `Object` or `any` but feel free to use them if you are spending too much time declaring types that depend on other types that depend on other types, and so on. diff --git a/src/core/types/coreTypes.js b/src/core/types/coreTypes.js index 32c399c7ab8..b94540b84a1 100644 --- a/src/core/types/coreTypes.js +++ b/src/core/types/coreTypes.js @@ -1,8 +1,14 @@ /* @flow */ /* global $Shape */ -// This is a workaround for exact object types when you can't use the pipe -// syntax: https://flowtype.org/en/docs/types/objects/#toc-exact-object-types +// TODO: This can go away when https://github.com/facebook/flow/issues/2405 +// is fixed. +// +// Exact is a fixed version of the $Exact object type, which you typically +// see used as pipes within an object type: +// https://flowtype.org/en/docs/types/objects/#toc-exact-object-types +// It is available as a generic utility called $Exact too: +// https://flow.org/en/docs/types/utilities/#toc-exact // // At the time of this writing, you may need to use this workaround if you // want to use the spread operator to merge objects together. See: From 04f6e84a5903dd699af656cdee3c7f23aa5ae479 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 17:45:31 -0500 Subject: [PATCH 65/71] alphabetize --- src/amo/actions/reviews.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/amo/actions/reviews.js b/src/amo/actions/reviews.js index bd76e304c5b..1085cd6ae29 100644 --- a/src/amo/actions/reviews.js +++ b/src/amo/actions/reviews.js @@ -7,10 +7,10 @@ export type UserReviewType = {| addonSlug: string, body: string, created: Date, - title: string, id: number, isLatest: boolean, rating: number, + title: string, userId: number, userName: string, userUrl: string, @@ -23,10 +23,10 @@ export function denormalizeReview(review: ApiReviewType): UserReviewType { addonSlug: review.addon.slug, body: review.body, created: review.created, - title: review.title, id: review.id, isLatest: review.is_latest, rating: review.rating, + title: review.title, userId: review.user.id, userName: review.user.name, userUrl: review.user.url, From 7a78042ed89ee253abf1561002c10cb6ac275d90 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Mon, 10 Apr 2017 17:49:10 -0500 Subject: [PATCH 66/71] removed type suffix from type files --- src/amo/components/RatingManager/index.js | 4 ++-- src/core/components/AuthenticateButton/index.js | 2 +- src/core/reducers/api.js | 2 +- src/core/types/{addonTypes.js => addons.js} | 0 src/core/types/{coreTypes.js => core.js} | 0 src/core/types/{reduxTypes.js => redux.js} | 0 6 files changed, 4 insertions(+), 4 deletions(-) rename src/core/types/{addonTypes.js => addons.js} (100%) rename src/core/types/{coreTypes.js => core.js} (100%) rename src/core/types/{reduxTypes.js => redux.js} (100%) diff --git a/src/amo/components/RatingManager/index.js b/src/amo/components/RatingManager/index.js index 67057ea43ca..10047f1dc2f 100644 --- a/src/amo/components/RatingManager/index.js +++ b/src/amo/components/RatingManager/index.js @@ -26,8 +26,8 @@ import type { UserReviewType } from 'amo/actions/reviews'; import type { SubmitReviewParams } from 'amo/api'; import type { UrlFormatParams } from 'core/api'; import type { ApiStateType } from 'core/reducers/api'; -import type { DispatchFunc } from 'core/types/reduxTypes'; -import type { AddonType, AddonVersionType } from 'core/types/addonTypes'; +import type { DispatchFunc } from 'core/types/redux'; +import type { AddonType, AddonVersionType } from 'core/types/addons'; import './styles.scss'; diff --git a/src/core/components/AuthenticateButton/index.js b/src/core/components/AuthenticateButton/index.js index 4d640c578c7..6ee06d1443a 100644 --- a/src/core/components/AuthenticateButton/index.js +++ b/src/core/components/AuthenticateButton/index.js @@ -12,7 +12,7 @@ import Button from 'ui/components/Button'; import Icon from 'ui/components/Icon'; import type { UrlFormatParams } from 'core/api'; import type { ApiStateType } from 'core/reducers/api'; -import type { DispatchFunc } from 'core/types/reduxTypes'; +import type { DispatchFunc } from 'core/types/redux'; type HandleLogInFunc = ( location: UrlFormatParams, options?: {| _window: typeof window |} diff --git a/src/core/reducers/api.js b/src/core/reducers/api.js index e95b8440f0a..a0f3d26e335 100644 --- a/src/core/reducers/api.js +++ b/src/core/reducers/api.js @@ -15,7 +15,7 @@ import type { SetLangAction, SetUserAgentAction, } from 'core/actions/index'; -import type { Exact } from 'core/types/coreTypes'; +import type { Exact } from 'core/types/core'; type UserAgentInfoType = {| browser: string, diff --git a/src/core/types/addonTypes.js b/src/core/types/addons.js similarity index 100% rename from src/core/types/addonTypes.js rename to src/core/types/addons.js diff --git a/src/core/types/coreTypes.js b/src/core/types/core.js similarity index 100% rename from src/core/types/coreTypes.js rename to src/core/types/core.js diff --git a/src/core/types/reduxTypes.js b/src/core/types/redux.js similarity index 100% rename from src/core/types/reduxTypes.js rename to src/core/types/redux.js From ee16518f1313b74d0b6f7116b12585fd03b33c8d Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 12 Apr 2017 10:13:06 -0500 Subject: [PATCH 67/71] rename -> core/types/util --- README.md | 2 +- src/core/reducers/api.js | 2 +- src/core/types/{core.js => util.js} | 0 3 files changed, 2 insertions(+), 2 deletions(-) rename src/core/types/{core.js => util.js} (100%) diff --git a/README.md b/README.md index 8bea9da0dcf..d0152a91e23 100644 --- a/README.md +++ b/README.md @@ -138,7 +138,7 @@ function getAllAddons({ categoryId }: GetAllAddonsParams = {}) { 'Inexact type is incompatible with exact type' but that's a [bug](https://github.com/facebook/flow/issues/2405). You can use the `Exact` workaround from - [`src/core/types/coreTypes`](https://github.com/mozilla/addons-frontend/blob/master/src/core/types/coreTypes.js) + [`src/core/types/util`](https://github.com/mozilla/addons-frontend/blob/master/src/core/types/util.js) if you have to. This is meant as a working replacement for [$Exact](https://flow.org/en/docs/types/utilities/#toc-exact). * Try to avoid loose types like `Object` or `any` but feel free to use diff --git a/src/core/reducers/api.js b/src/core/reducers/api.js index a0f3d26e335..b95fd1e4dde 100644 --- a/src/core/reducers/api.js +++ b/src/core/reducers/api.js @@ -15,7 +15,7 @@ import type { SetLangAction, SetUserAgentAction, } from 'core/actions/index'; -import type { Exact } from 'core/types/core'; +import type { Exact } from 'core/types/util'; type UserAgentInfoType = {| browser: string, diff --git a/src/core/types/core.js b/src/core/types/util.js similarity index 100% rename from src/core/types/core.js rename to src/core/types/util.js From 7dd4d3e65696cfa9b8cac4457a976c816f7ef45c Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 12 Apr 2017 10:13:59 -0500 Subject: [PATCH 68/71] capitalize GitHub --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index d0152a91e23..79dd60db2f7 100644 --- a/README.md +++ b/README.md @@ -148,7 +148,7 @@ function getAllAddons({ categoryId }: GetAllAddonsParams = {}) { into a bug or if you hit something that's making you bang your head on the keyboard. If it's something you think is unfixable then use `$FLOW_IGNORE` instead. Please explain your rationale in the comment and link - to a github issue if possible. + to a GitHub issue if possible. ### Code coverage From 2c3d08ce8bdc8397ce4dc02c12a583dd5f875c86 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 12 Apr 2017 10:21:13 -0500 Subject: [PATCH 69/71] rename data -> review --- src/amo/api/index.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/amo/api/index.js b/src/amo/api/index.js index bf4e3d39aa7..94b573de7b9 100644 --- a/src/amo/api/index.js +++ b/src/amo/api/index.js @@ -53,7 +53,7 @@ export function submitReview({ }: SubmitReviewParams): Promise { return new Promise( (resolve) => { - const data = { + const review = { addon: undefined, rating, version: versionId, @@ -67,17 +67,17 @@ export function submitReview({ endpoint = `${endpoint}/${reviewId}`; method = 'PATCH'; // You cannot update the version of an existing review. - data.version = undefined; + review.version = undefined; } else { if (!addonId) { throw new Error('addonId is required when posting a new review'); } - data.addon = addonId; + review.addon = addonId; } resolve(callApi({ endpoint, - body: data, + body: review, method, auth: true, state: apiState, From f3794315913a1c296e6a64df88df62d53f504ff8 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 12 Apr 2017 10:29:45 -0500 Subject: [PATCH 70/71] alphabetize all type params --- src/amo/api/index.js | 10 +++++----- src/core/actions/index.js | 12 ++++++------ src/core/api/index.js | 26 +++++++++++++------------- src/core/reducers/api.js | 8 ++++---- src/core/types/addons.js | 8 ++++---- 5 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/amo/api/index.js b/src/amo/api/index.js index 94b573de7b9..288bc4adddb 100644 --- a/src/amo/api/index.js +++ b/src/amo/api/index.js @@ -11,10 +11,10 @@ export type ApiReviewType = {| |}, body: string, created: Date, - title: string, id: number, is_latest: boolean, rating: number, + title: string, user: {| id: number, name: string, @@ -29,13 +29,13 @@ export type ApiReviewType = {| // can type check each one independently. export type SubmitReviewParams = {| addonId?: number, - rating?: number, apiState?: ApiStateType, + body?: string, errorHandler?: ErrorHandlerType, + rating?: number, + reviewId?: number, title?: string, versionId?: number, - body?: string, - reviewId?: number, |}; /* @@ -113,8 +113,8 @@ export function getReviews( } type GetLatestReviewParams = {| - user: number, addon: number, + user: number, |}; export function getLatestUserReview( diff --git a/src/core/actions/index.js b/src/core/actions/index.js index 16a41e43451..d420216488e 100644 --- a/src/core/actions/index.js +++ b/src/core/actions/index.js @@ -10,8 +10,8 @@ import { } from 'core/constants'; export type SetAuthTokenAction = {| - type: string, payload: {| token: string |}, + type: string, |}; export function setAuthToken(token: string): SetAuthTokenAction { @@ -33,8 +33,8 @@ export function logOutUser(): LogOutUserAction { } export type SetClientAppAction = {| - type: string, payload: {| clientApp: string |}, + type: string, |}; export function setClientApp(clientApp: string): SetClientAppAction { @@ -48,8 +48,8 @@ export function setClientApp(clientApp: string): SetClientAppAction { } export type SetLangAction = {| - type: string, payload: {| lang: string |}, + type: string, |}; export function setLang(lang: string): SetLangAction { @@ -60,8 +60,8 @@ export function setLang(lang: string): SetLangAction { } export type SetUserAgentAction = {| - type: string, payload: {| userAgent: string |}, + type: string, |}; export function setUserAgent(userAgent: string): SetUserAgentAction { @@ -72,8 +72,8 @@ export function setUserAgent(userAgent: string): SetUserAgentAction { } export type LoadEntitiesAction = {| - type: string, payload: {| entities: Array |}, + type: string, |}; export function loadEntities(entities: Array): LoadEntitiesAction { @@ -84,8 +84,8 @@ export function loadEntities(entities: Array): LoadEntitiesAction { } export type SetCurrentUserAction = {| - type: string, payload: {| username: string |}, + type: string, |}; export function setCurrentUser(username: string): SetCurrentUserAction { diff --git a/src/core/api/index.js b/src/core/api/index.js index fe1ba7869e3..9cd9cbf0e61 100644 --- a/src/core/api/index.js +++ b/src/core/api/index.js @@ -64,15 +64,15 @@ export function createApiError( } type CallApiParams = {| - endpoint: string, - schema?: Object, - params?: Object, auth?: boolean, - state?: ApiStateType, - method?: 'GET' | 'POST' | 'DELETE' | 'HEAD' | 'OPTIONS' | 'PUT' | 'PATCH', body?: Object, credentials?: boolean, + endpoint: string, errorHandler?: ErrorHandlerType, + method?: 'GET' | 'POST' | 'DELETE' | 'HEAD' | 'OPTIONS' | 'PUT' | 'PATCH', + params?: Object, + schema?: Object, + state?: ApiStateType, |}; export function callApi({ @@ -161,9 +161,9 @@ export function callApi({ type SearchParams = {| api: ApiStateType, - page: number, auth: boolean, filters: Object, + page: number, |}; export function search( @@ -240,17 +240,17 @@ export function login({ api, code, state }: LoginParams) { // These are all possible parameters to url.format() export type UrlFormatParams = {| - +href?: string; - +protocol?: string; - +slashes?: boolean; +auth?: string; - +hostname?: string; - +port?: string | number; + +hash?: string; +host?: string; + +hostname?: string; + +href?: string; +pathname?: string; - +search?: string; + +port?: string | number; + +protocol?: string; +query?: Object; - +hash?: string; + +search?: string; + +slashes?: boolean; |}; export function startLoginUrl({ location }: { location: UrlFormatParams }) { diff --git a/src/core/reducers/api.js b/src/core/reducers/api.js index b95fd1e4dde..bfe4dc50a49 100644 --- a/src/core/reducers/api.js +++ b/src/core/reducers/api.js @@ -23,17 +23,17 @@ type UserAgentInfoType = {| |}; export type ApiStateType = { - token: ?string, - lang: ?string, clientApp: ?string, + lang: ?string, + token: ?string, userAgent: ?string, userAgentInfo: ?UserAgentInfoType, }; export const initialApiState = { - token: null, - lang: null, clientApp: null, + lang: null, + token: null, userAgent: null, userAgentInfo: null, }; diff --git a/src/core/types/addons.js b/src/core/types/addons.js index 607efd3feb5..4c3df88463f 100644 --- a/src/core/types/addons.js +++ b/src/core/types/addons.js @@ -1,9 +1,9 @@ /* @flow */ export type AddonVersionType = {| - id: number, channel: string, edit_url: string, files: Array, + id: number, // The `text` property is omitted from addon.current_version.license. license: { name: string, url: string }, reviewed: Date, @@ -16,7 +16,6 @@ export type AddonAuthorType = {| |}; export type AddonType = {| - id: number, authors: Array, average_daily_users: number, categories: Object, @@ -30,17 +29,18 @@ export type AddonType = {| has_privacy_policy: boolean, homepage: string, icon_url: string, + id: number, is_disabled: boolean, is_experimental: boolean, is_source_public: boolean, - name: string, last_updated: Date, latest_unlisted_version: ?AddonVersionType, + name: string, previews: Array, public_stats: boolean, ratings: {| - count: number, average: number, + count: number, |}, review_url: string, slug: string, From 3f512df5dc55ed21f3a3e0a3f77a85a626d882a1 Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Wed, 12 Apr 2017 14:03:39 -0500 Subject: [PATCH 71/71] add leading pipe for pretty formatting --- src/core/types/addons.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/types/addons.js b/src/core/types/addons.js index 4c3df88463f..6c84a9011d8 100644 --- a/src/core/types/addons.js +++ b/src/core/types/addons.js @@ -44,7 +44,8 @@ export type AddonType = {| |}, review_url: string, slug: string, - status: 'beta' + status: + | 'beta' | 'lite' | 'public' | 'deleted'