Skip to content

Commit

Permalink
refactor: improve typings (#45)
Browse files Browse the repository at this point in the history
* Add type guard, generic, and test

* Improve generic naming

* Add ErrorFSA alias

* Write test correctly to avoid confusion

* Add `someField` validation

* Add somefield test

* Add semi-colon, move tsc to posttest

* Add 3 more semi-colon

* Remove optional designation.

Since `payload` and `meta` is generic, user can specify `void` or `undefined` to indicate it does not present.

Removing the optional designation improves type guard.

see `isCustomAction4()` for example

* Fix more linting issue

* Adding eslint for typescript. Update yarn.lock

* Change typescript-eslint-parser to devDeps

* Move eslint-plugin-typescirpt to devDeps also.
  • Loading branch information
unional authored and JaKXz committed Jan 24, 2017
1 parent 17b0b04 commit 6a2898a
Show file tree
Hide file tree
Showing 6 changed files with 145 additions and 9 deletions.
4 changes: 4 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ module.exports = {
"globals": {
"expect": true
},
"parser": "typescript-eslint-parser",
"plugins": [
"typescript"
],
"rules": {
"no-use-before-define": ["error", "nofunc"],
"no-unused-expressions": 0,
Expand Down
8 changes: 6 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
"lint": "eslint src/ test/",
"prepublish": "npm test && npm run build",
"pretest": "npm run lint",
"test": "cross-env NODE_ENV=test nyc mocha"
"test": "cross-env NODE_ENV=test nyc mocha",
"posttest": "tsc"
},
"repository": {
"type": "git",
Expand All @@ -41,9 +42,12 @@
"eslint": "^3.10.2",
"eslint-config-airbnb-base": "^11.0.1",
"eslint-plugin-import": "^2.2.0",
"eslint-plugin-typescript": "^0.1.0",
"mocha": "^3.1.2",
"nyc": "^10.0.0",
"rimraf": "^2.5.4"
"rimraf": "^2.5.4",
"typescript": "^2.0.10",
"typescript-eslint-parser": "^1.0.2"
},
"dependencies": {
"lodash.isplainobject": "^4.0.6",
Expand Down
23 changes: 16 additions & 7 deletions src/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export interface FluxStandardAction {
export interface FluxStandardAction<Payload, Meta> {
/**
* The `type` of an action identifies to the consumer the nature of the action that has occurred.
* Two actions with the same `type` MUST be strictly equivalent (using `===`)
Expand All @@ -11,7 +11,7 @@ export interface FluxStandardAction {
* By convention, if `error` is `true`, the `payload` SHOULD be an error object.
* This is akin to rejecting a promise with an error object.
*/
payload?: any;
payload: Payload;
/**
* The optional `error` property MAY be set to true if the action represents an error.
* An action whose `error` is true is analogous to a rejected Promise.
Expand All @@ -23,20 +23,29 @@ export interface FluxStandardAction {
* The optional `meta` property MAY be any type of value.
* It is intended for any extra information that is not part of the payload.
*/
meta?: any
meta: Meta;
}

export interface ErrorFluxStandardAction<CustomError extends Error, Meta> extends FluxStandardAction<CustomError, Meta> {
error: true;
}

/**
* Alias for FluxStandardAction.
*/
export type FSA<Payload, Meta> = FluxStandardAction<Payload, Meta>;

/**
* Alias to FluxStandardAction for shorthand
* Alias for ErrorFluxStandardAction.
*/
export type FSA = FluxStandardAction;
export type ErrorFSA<CustomError extends Error, Meta> = ErrorFluxStandardAction<CustomError, Meta>;

/**
* Returns `true` if `action` is FSA compliant.
*/
export function isFSA(action: any): boolean;
export function isFSA<Payload, Meta>(action: any): action is FluxStandardAction<Payload, Meta>;

/**
* Returns `true` if `action` is FSA compliant error.
*/
export function isError(action: any): boolean;
export function isError<CustomError extends Error, Meta>(action: any): action is ErrorFluxStandardAction<CustomError, Meta>;
78 changes: 78 additions & 0 deletions test/typings-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { FluxStandardAction, isError, isFSA } from '../src';

interface CustomPayload {
a: number;
}

interface CustomMetadata {
b: string;
}

interface MyError extends Error {
someField: string;
}

function createCustomAction(payload: CustomPayload) {
return {
type: 'custom',
payload
};
}

function isCustomAction(action: FluxStandardAction<any, any>): action is FluxStandardAction<CustomPayload, any> {
return isFSA(action) && action.type === 'custom';
}

function isCustomAction2(action: FluxStandardAction<any, any>): action is FluxStandardAction<CustomPayload, CustomMetadata> {
return isFSA(action) && action.type === 'custom2';
}

function isCustomAction3(action: any): action is FluxStandardAction<void, string> {
return isFSA(action) && action.type === 'custom3';
}

function isCustomAction4(action: any): action is FluxStandardAction<{ message: string }, void> {
return true;
}

let action2 = {};
if (isCustomAction4(action2)) {
// type guard infers payload will not be undefined
console.log(action2.payload.message);
}

function reducer(state, action) {
if (isFSA<CustomPayload, void>(action)) {
let a: number = action.payload.a;
}
else if (isFSA<CustomPayload, CustomMetadata>(action)) {
let a: number = action.payload.a;
let b: string = action.meta.b;
}
else if (isFSA<void, string>(action)) {
let meta: string = action.meta;
}
else if (isError(action)) {
let iserr: true = action.error; // iserr === true
let err: Error = action.payload;
}
else if (isError<MyError, void>(action)) {
let err: MyError = action.payload;
let someFieldValue: string = err.someField;
}
}

function reducer2(state, action) {
if (isCustomAction(action)) {
let a: number = action.payload.a;
}
else if (isCustomAction2(action)) {
let a: number = action.payload.a;
let b: string = action.meta.b;
}
else if (isCustomAction3(action)) {
let meta: string = action.meta;
}
}

let action = createCustomAction({ a: 123 });
10 changes: 10 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"compilerOptions": {
"noEmit": true,
"strictNullChecks": true,
"target": "es5"
},
"files": [
"test/typings-test.ts"
]
}
31 changes: 31 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1258,6 +1258,12 @@ eslint-plugin-import@^2.2.0:
minimatch "^3.0.3"
pkg-up "^1.0.0"

eslint-plugin-typescript@^0.1.0:
version "0.1.0"
resolved "https://registry.yarnpkg.com/eslint-plugin-typescript/-/eslint-plugin-typescript-0.1.0.tgz#6d82b7960f3ff21a499e8b157b304453be5a5700"
dependencies:
requireindex "~1.1.0"

eslint@^3.10.2:
version "3.13.0"
resolved "https://registry.yarnpkg.com/eslint/-/eslint-3.13.0.tgz#636925fd163c9babe2e8be7ae43caf518d469577"
Expand Down Expand Up @@ -2079,6 +2085,16 @@ lodash.pickby@^4.6.0:
version "4.6.0"
resolved "https://registry.yarnpkg.com/lodash.pickby/-/lodash.pickby-4.6.0.tgz#7dea21d8c18d7703a27c704c15d3b84a67e33aff"

lodash.tostring@^4.0.0:
version "4.1.4"
resolved "https://registry.yarnpkg.com/lodash.tostring/-/lodash.tostring-4.1.4.tgz#560c27d1f8eadde03c2cce198fef5c031d8298fb"

lodash.unescape@4.0.0:
version "4.0.0"
resolved "https://registry.yarnpkg.com/lodash.unescape/-/lodash.unescape-4.0.0.tgz#36debfc492b81478471ef974cd3783e202eb6cef"
dependencies:
lodash.tostring "^4.0.0"

lodash@^4.0.0, lodash@^4.2.0, lodash@^4.3.0:
version "4.17.4"
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.4.tgz#78203a4d1c328ae1d86dca6460e369b57f4055ae"
Expand Down Expand Up @@ -2674,6 +2690,10 @@ require-uncached@^1.0.2:
caller-path "^0.1.0"
resolve-from "^1.0.0"

requireindex@~1.1.0:
version "1.1.0"
resolved "https://registry.yarnpkg.com/requireindex/-/requireindex-1.1.0.tgz#e5404b81557ef75db6e49c5a72004893fe03e162"

resolve-from@^1.0.0:
version "1.0.1"
resolved "https://registry.yarnpkg.com/resolve-from/-/resolve-from-1.0.1.tgz#26cbfe935d1aeeeabb29bc3fe5aeb01e93d44226"
Expand Down Expand Up @@ -2972,6 +2992,17 @@ typedarray@^0.0.6:
version "0.0.6"
resolved "https://registry.yarnpkg.com/typedarray/-/typedarray-0.0.6.tgz#867ac74e3864187b1d3d47d996a78ec5c8830777"

typescript-eslint-parser@^1.0.2:
version "1.0.2"
resolved "https://registry.yarnpkg.com/typescript-eslint-parser/-/typescript-eslint-parser-1.0.2.tgz#fd2abacf2ee3d9382ab3e449c8762b6beae4d0d7"
dependencies:
lodash.unescape "4.0.0"
object-assign "^4.0.1"

typescript@^2.0.10:
version "2.1.5"
resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.1.5.tgz#6fe9479e00e01855247cea216e7561bafcdbcd4a"

uglify-js@^2.6:
version "2.7.5"
resolved "https://registry.yarnpkg.com/uglify-js/-/uglify-js-2.7.5.tgz#4612c0c7baaee2ba7c487de4904ae122079f2ca8"
Expand Down

6 comments on commit 6a2898a

@michabu
Copy link

Choose a reason for hiding this comment

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

Hi @unional, I noticed that you changed the type of meta from optional meta?: any to required meta: Meta.

I think that might be a mistake. If not: Can you please explain the intent?

@unional
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the question.

The original comment on that was here #45 (comment)
and
microsoft/TypeScript#12400

The idea is that when Payload and Meta is defined by a custom action, the action author would expect the payload and meta to be non-optional.

If defined as meta?, then the user always have to check if it is not undefined on every usage.

@michabu
Copy link

@michabu michabu commented on 6a2898a Feb 3, 2017

Choose a reason for hiding this comment

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

Hi @unional,

How would I define a FluxStandardAction without a Meta? I cannot find an according overload.

All my Actions (at the moment) have a Payload but not all of them need a Meta. Maybe there even could be actions that wouldn't need a payload? Would that be possible?

@unional
Copy link
Contributor Author

@unional unional commented on 6a2898a Feb 3, 2017

Choose a reason for hiding this comment

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

Just do FSA<MyPayload, undefined>. When generic default is available, it can be simplified back to FSA<MyPayload>. Separating Meta<M> on its own (as in the typing in DT) is interesting but it feels like a hack, making consuming code more cluttered IMO.

Hope that helps

@michabu
Copy link

@michabu michabu commented on 6a2898a Feb 3, 2017

Choose a reason for hiding this comment

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

I tried FSA<MyPayload, undefined> and it kept telling me there was an error, so I had to add

interface MyAction {
  …,
  meta: undefined
}

to my Action. What am I missing?

@unional
Copy link
Contributor Author

@unional unional commented on 6a2898a Feb 3, 2017

Choose a reason for hiding this comment

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

Yes. See the TypeScript issue

Please sign in to comment.