Improve typings #45

unional · 2016-11-18T22:42:53Z

I have added a few things:

Add type guard
Add generic types to the interface. The main usage is on isFSA and isError
Add test

This allows users to specify the type of the payload and meta easily.
See typings-test.ts for examples.

In the future, I would like to make the Meta type optional.
But need to wait for microsoft/TypeScript#2175

For now, I think this is the best it can get. 🌷

coveralls · 2016-11-18T22:46:33Z

Coverage remained the same at 100.0% when pulling 421cac2 on unional:master into d9b70df on acdlite:master.

coveralls · 2016-11-18T22:53:07Z

Coverage remained the same at 100.0% when pulling 133fbdf on unional:master into d9b70df on acdlite:master.

coveralls · 2016-11-18T22:53:07Z

Coverage remained the same at 100.0% when pulling 133fbdf on unional:master into d9b70df on acdlite:master.

coveralls · 2016-11-18T23:17:36Z

Coverage remained the same at 100.0% when pulling 5633fff on unional:master into d9b70df on acdlite:master.

coveralls · 2016-11-18T23:17:36Z

Coverage remained the same at 100.0% when pulling 5633fff on unional:master into d9b70df on acdlite:master.

coveralls · 2016-11-18T23:17:36Z

Coverage remained the same at 100.0% when pulling 5633fff on unional:master into d9b70df on acdlite:master.

coveralls · 2016-11-18T23:18:52Z

Coverage remained the same at 100.0% when pulling f542e79 on unional:master into d9b70df on acdlite:master.

coveralls · 2016-11-18T23:18:53Z

Coverage remained the same at 100.0% when pulling f542e79 on unional:master into d9b70df on acdlite:master.

JaKXz

Thanks for the PR! Made some minor comments about style and being consistent with the rest of the repo (please look over typings-test.ts again), but I'll look more deeply later.

JaKXz · 2016-11-19T00:41:08Z

package.json

@@ -14,7 +14,7 @@
    "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 && tsc"


Please use the posttest lifecycle hook for tsc.

Nice, never use posttest before. :)

JaKXz · 2016-11-19T00:43:12Z

test/typings-test.ts

@@ -0,0 +1,68 @@
+import { FluxStandardAction, isError, isFSA } from '../src'


I would prefer to see semi-colons to be consistent with index.d.ts and the JS files.

coveralls · 2016-11-19T03:53:26Z

Coverage remained the same at 100.0% when pulling 31f0e76 on unional:master into d9b70df on acdlite:master.

coveralls · 2016-11-19T03:53:26Z

Coverage remained the same at 100.0% when pulling 31f0e76 on unional:master into d9b70df on acdlite:master.

unional · 2016-11-19T03:54:50Z

Updated based on feedback. 🌷

coveralls · 2016-11-19T04:03:26Z

Coverage remained the same at 100.0% when pulling 22229d3 on unional:master into d9b70df on acdlite:master.

coveralls · 2016-11-21T05:52:45Z

Coverage remained the same at 100.0% when pulling 78a9065 on unional:master into d9b70df on acdlite:master.

coveralls · 2016-11-21T05:52:46Z

Coverage remained the same at 100.0% when pulling 78a9065 on unional:master into d9b70df on acdlite:master.

unional · 2016-11-21T06:40:32Z

The last change I made worth debating. On one side it helps on the type guard usage, on the other side it makes action creation harder:
microsoft/TypeScript#12400

What do you think?

unional · 2016-11-21T07:29:53Z

I think it is the right change. When payload and meta is defined by a custom action, the action author would expect the payload and meta to be non-optional.

Would that be any case that this is not true?

unional · 2016-11-21T07:35:42Z

cc/ @tkqubo who add typings to DT.

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

coveralls · 2016-12-11T04:53:31Z

Coverage remained the same at 100.0% when pulling c9fa687 on unional:master into 0c3752a on acdlite:master.

unional · 2016-12-12T01:12:29Z

@JaKXz do you have anything for me to change before we can merge this? 🌷

JaKXz

@unional so sorry for the delay! I finally have some OSS cycles again. 🌹

I have a couple more notes:

please run yarn to update yarn.lock
the posttest action that runs tsc will return an exit code of 1 if there is a problem with the typings, correct?

There are some more nitpicks I could make in typings-test.ts about code style [e.g. missing semicolons 😱 ] - can that file [and other .ts files in the repo] be linted somehow?

And last question, what is the status on the typescript issue you posted earlier? I saw that it's still open and was updated recently, but it's a long thread so I haven't read it closely yet. Will that be a fix that comes in later or does it block this PR?

unional · 2017-01-09T07:17:37Z

@JaKXz good to hear from you!

here are some more nitpicks I could make in typings-test.ts about code style [e.g. missing semicolons 😱 ] - can that file [and other .ts files in the repo] be linted somehow?

It still have? Seems like I'm getting very comfortable with my own linting standard 😛 .
I can use tslint (which I do for my projects), but the key is getting the same linting config as your are comfortable with.
Let me know what do you want to fix (other than the semicolons), and I will fix them.

the posttest action that runs tsc will return an exit code of 1 if there is a problem with the typings, correct?

Yes, but since it is posttest, I don't think it will stop the build.

please run yarn to update yarn.lock

I personally do not recommend checking in yarn.lock for module library. It makes breakage detection harder for the maintainer. I can do it if you still want to.

what is the status on the typescript issue

They are working on it, likely will still take a few months to land. We can update the typing when that is released and get mainstreamed.

coveralls · 2017-01-09T07:26:41Z

Coverage remained the same at 100.0% when pulling 5216925 on unional:master into 0c3752a on acdlite:master.

unional · 2017-01-09T07:30:04Z

Found the comment I made about yarn.lock: greenkeeperio/greenkeeper#314 (comment)

JaKXz

@unional re: yarn.lock - as a maintainer it has actually made my life much easier and helped me catch a number of issues regarding deps before merging PRs. I think the purpose of the yarn.lock file is to give some reassurance about what versions of dependencies and sub-dependencies are resolved & installed. Having a way of knowing which dependencies are installed, and then being able to investigate why certain dependencies are installed at certain versions are reassurances we haven't had before with node modules.

It makes breakage detection harder for the maintainer.

I disagree with this statement. I am able to trace the history of my dependencies (and their dependencies) in a git bisect. I am going to ask that you update the yarn.lock before I can merge this PR.

re: linting - I'm just considering the cost of maintenance [so if people want to contribute it the future, how can we make it so that they can get feedback about code style from a robot rather than me having to comment on every line of their changes]. Do you know of a houndCI for TypeScript? Or would we have to add those dependencies to the project [which I don't really prefer since this is just a generic JS project]?

Also, it would be ideal if the tsc command exited with a non-zero code when the compilation fails so that there's feedback that the typings have broken. I'm pretty sure if posttest fails, any commands that depend on test will not execute.

JaKXz · 2017-01-10T21:31:28Z

perhaps https://github.com/nzakas/eslint-plugin-typescript would help?

coveralls · 2017-01-23T23:27:46Z

Coverage remained the same at 100.0% when pulling b9d81be on unional:master into 00c2b9b on acdlite:master.

coveralls · 2017-01-23T23:27:46Z

Coverage remained the same at 100.0% when pulling b9d81be on unional:master into 00c2b9b on acdlite:master.

coveralls · 2017-01-23T23:27:46Z

Coverage remained the same at 100.0% when pulling b9d81be on unional:master into 00c2b9b on acdlite:master.

unional · 2017-01-24T00:15:11Z

it would be ideal if the tsc command exited with a non-zero code

It does exist with non-zero. I was just concerning that posttest non-zero exit will be ignored. Seems like it will not be the case.

I think the purpose of the yarn.lock file is to give some reassurance about what versions of dependencies and sub-dependencies are resolved & installed

yes, that's true for applications. For modules/packages, IMO it will hide the problem until a user reports it. E.g. if your module depends on [email protected] and unfortunately [email protected] breaks your code, you won't know it until some users use your module and discover it is broken.

This is because during consumption, the yarn.lock file of your module is not used to lock sub-dependency.

The article (https://yarnpkg.com/blog/2016/11/24/lockfiles-for-all) about devDeps vs deps is correct thou.

All in all, maybe this topic is still in flux and it will clear out as more module authors adopt one way or the other. This is just a friendly discussion. 🌷

Maybe using yarn.lock with greenkeeper would work out just fine?

I have updated the yarn.lock file anyway, as you requested. 🌷

JaKXz · 2017-01-24T00:53:38Z

package.json

    "lodash.isplainobject": "^4.0.6",
    "lodash.isstring": "^4.0.1",
-    "lodash.issymbol": "^4.0.1"
+    "lodash.issymbol": "^4.0.1",
+    "typescript-eslint-parser": "^1.0.2"


I think all typescript dependencies should be in devDeps.

Yes, of course. 😛

JaKXz · 2017-01-24T00:57:07Z

@unional thank you for your thoughts, hard work, and patience on this. I'm excited to get it merged after one last issue and release v1.1.0 :)

coveralls · 2017-01-24T01:13:35Z

Coverage remained the same at 100.0% when pulling 73529c4 on unional:master into 00c2b9b on acdlite:master.

coveralls · 2017-01-24T01:13:35Z

Coverage remained the same at 100.0% when pulling 73529c4 on unional:master into 00c2b9b on acdlite:master.

coveralls · 2017-01-24T01:13:35Z

Coverage remained the same at 100.0% when pulling 73529c4 on unional:master into 00c2b9b on acdlite:master.

JaKXz · 2017-01-24T01:20:47Z

package.json

  },
  "dependencies": {
+    "eslint-plugin-typescript": "^0.1.0",


missed this one as well

coveralls · 2017-01-24T02:50:59Z

Coverage remained the same at 100.0% when pulling 4157c98 on unional:master into 00c2b9b on acdlite:master.

coveralls · 2017-01-24T02:50:59Z

Coverage remained the same at 100.0% when pulling 4157c98 on unional:master into 00c2b9b on acdlite:master.

coveralls · 2017-01-24T02:50:59Z

Coverage remained the same at 100.0% when pulling 4157c98 on unional:master into 00c2b9b on acdlite:master.

unional mentioned this pull request Nov 18, 2016

Delete flux-standard-action.json typings/registry#910

Open

JaKXz reviewed Nov 19, 2016

View reviewed changes

unional mentioned this pull request Nov 21, 2016

Treating undefined parameters as optional microsoft/TypeScript#12400

Open

unional added 9 commits December 10, 2016 20:51

Add type guard, generic, and test

5eb7a9c

Improve generic naming

9d1c497

Add ErrorFSA alias

ce57e63

Write test correctly to avoid confusion

644622e

Add someField validation

80a71fa

Add somefield test

2a6b80a

Add semi-colon, move tsc to posttest

8c64c1b

Add 3 more semi-colon

ee78552

Remove optional designation.

c9fa687

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

unional force-pushed the master branch from 78a9065 to c9fa687 Compare December 11, 2016 04:52

JaKXz reviewed Jan 9, 2017

View reviewed changes

Fix more linting issue

5216925

JaKXz suggested changes Jan 10, 2017

View reviewed changes

JaKXz self-assigned this Jan 10, 2017

unional added 2 commits January 23, 2017 15:19

Merge remote-tracking branch 'acdlite/master'

3df9bea

Adding eslint for typescript. Update yarn.lock

b9d81be

JaKXz reviewed Jan 24, 2017

View reviewed changes

JaKXz requested a review from acdlite January 24, 2017 00:57

Change typescript-eslint-parser to devDeps

73529c4

JaKXz reviewed Jan 24, 2017

View reviewed changes

package.json

},

"dependencies": {

"eslint-plugin-typescript": "^0.1.0",

Copy link

Contributor

JaKXz Jan 24, 2017

Choose a reason for hiding this comment

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

missed this one as well

Move eslint-plugin-typescirpt to devDeps also.

4157c98

JaKXz merged commit 6a2898a into redux-utilities:master Jan 24, 2017

		@@ -0,0 +1,68 @@
		import { FluxStandardAction, isError, isFSA } from '../src'

Improve typings #45

Improve typings #45

Conversation

unional commented Nov 18, 2016 • edited Loading

coveralls commented Nov 18, 2016 • edited Loading

coveralls commented Nov 18, 2016

coveralls commented Nov 18, 2016 • edited Loading

coveralls commented Nov 18, 2016

coveralls commented Nov 18, 2016

coveralls commented Nov 18, 2016

coveralls commented Nov 18, 2016

coveralls commented Nov 18, 2016 • edited Loading

JaKXz left a comment

Choose a reason for hiding this comment

JaKXz Nov 19, 2016

Choose a reason for hiding this comment

unional Nov 19, 2016

Choose a reason for hiding this comment

JaKXz Nov 19, 2016

Choose a reason for hiding this comment

unional Nov 19, 2016

Choose a reason for hiding this comment

coveralls commented Nov 19, 2016

coveralls commented Nov 19, 2016 • edited Loading

unional commented Nov 19, 2016

coveralls commented Nov 19, 2016 • edited Loading

coveralls commented Nov 21, 2016 • edited Loading

coveralls commented Nov 21, 2016

unional commented Nov 21, 2016

unional commented Nov 21, 2016

unional commented Nov 21, 2016

coveralls commented Dec 11, 2016 • edited Loading

unional commented Dec 12, 2016

JaKXz left a comment • edited Loading

Choose a reason for hiding this comment

unional commented Jan 9, 2017

coveralls commented Jan 9, 2017 • edited Loading

unional commented Jan 9, 2017

JaKXz left a comment • edited Loading

Choose a reason for hiding this comment

JaKXz commented Jan 10, 2017

coveralls commented Jan 23, 2017

coveralls commented Jan 23, 2017

coveralls commented Jan 23, 2017

unional commented Jan 24, 2017 • edited Loading

JaKXz Jan 24, 2017

Choose a reason for hiding this comment

unional Jan 24, 2017

Choose a reason for hiding this comment

JaKXz commented Jan 24, 2017

coveralls commented Jan 24, 2017

coveralls commented Jan 24, 2017

coveralls commented Jan 24, 2017

JaKXz Jan 24, 2017

Choose a reason for hiding this comment

coveralls commented Jan 24, 2017

coveralls commented Jan 24, 2017

coveralls commented Jan 24, 2017

unional commented Nov 18, 2016 •

edited

Loading

coveralls commented Nov 18, 2016 •

edited

Loading

coveralls commented Nov 18, 2016 •

edited

Loading

coveralls commented Nov 18, 2016 •

edited

Loading

coveralls commented Nov 19, 2016 •

edited

Loading

coveralls commented Nov 19, 2016 •

edited

Loading

coveralls commented Nov 21, 2016 •

edited

Loading

coveralls commented Dec 11, 2016 •

edited

Loading

JaKXz left a comment •

edited

Loading

coveralls commented Jan 9, 2017 •

edited

Loading

JaKXz left a comment •

edited

Loading

unional commented Jan 24, 2017 •

edited

Loading