Add typings to the repo #33

unional · 2016-06-28T22:41:26Z

Would you be interested in accepting this PR so TypeScript user can get the typings definition directly?

Thanks. 🌷

jayphelps · 2016-09-09T21:11:26Z

JaKXz

@unional this LGTM, thanks for the contribution! A major overhaul was recently merged so would you mind rebasing and re-generating the typings file? Thanks.

coveralls · 2016-10-27T05:47:15Z

Coverage remained the same at 100.0% when pulling cd45da8 on unional:master into 08fdb1b on acdlite:master.

unional · 2016-10-27T05:50:00Z

I have updated it.

They typings file is basically the same. I just further clean up the comments as now VSC supports tilde in the intelliSense.

coveralls · 2016-10-27T05:53:26Z

Coverage remained the same at 100.0% when pulling 26a7bf3 on unional:master into 08fdb1b on acdlite:master.

coveralls · 2016-10-27T05:53:26Z

Coverage remained the same at 100.0% when pulling 26a7bf3 on unional:master into 08fdb1b on acdlite:master.

coveralls · 2016-10-27T05:53:26Z

Coverage remained the same at 100.0% when pulling 26a7bf3 on unional:master into 08fdb1b on acdlite:master.

JaKXz · 2016-10-27T06:04:34Z

package.json

@@ -3,6 +3,7 @@
  "version": "0.6.1",
  "description": "A human-friendly standard for Flux action objects",
  "main": "lib/index.js",
+  "typings": "src/index.d.ts",
  "files": [
    "lib"


Doesn't the index.d.ts need to be in the published files? I would rather you move this to the root so that this array can just be:

[ "lib", "index.d.ts" ]

We can do that. Putting the file directly next to the source allows editor (VS Code) to pick it up automatically.

JaKXz · 2016-10-27T06:06:30Z

src/index.d.ts

+   * 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.
+   * By convention, the `payload` SHOULD be an error object.
+   * If `error` has any other value besides `true`, including `undefined` and `null`, the action MUST NOT be interpreted as an error.


I'm not sure if this last statement is [strictly] true, since the tests have a line where the error object is an instanceof Error. see: https://github.com/acdlite/flux-standard-action/blob/master/test/isFSA-test.js#L31

This is directly from the README. Do you want to update that?

Good point - it should be updated.

Though which one is defined as correct is still debatable imho [b/c we have a test enforcing one way and documentation mentioning another]. Thoughts @acdlite?

cc @yangmillstheory for your thoughts?

I think more people have probably read the README than studied the test - at least that's my personal experience with this. I would vote for aligning the test with the documentation, but the ultimate goal would be to pick one of those sources of truth and stick with it.

Also, it's been a while since I've done TypeScript, but shouldn't this be boolean, since no other value is meaningful?

I would like to keep it as simpleboolean. But by README, I assume you allow it to be other values.

It's "allowed", but not meaningful. In TypeScript we should have this be

type?: boolean

so that users know at compile-time that the only meaningful values are true, false, undefined, or null.

I'm not sure how to exclude null, since ? seems to allow it:

interface Foo { foo?: boolean } const foo: Foo = { foo: null } // no compiler errors

When user enables strictNullCheck, it is excluded from ?.

Is there a way to disambiguate between null and undefined in code? I think 2.0 has Null and Undefined.

unional · 2016-10-28T18:18:31Z

@JaKXz I update package.json to instruct babel to copy the file over to lib, so the editor can recognize the typings and it is still distributed under lib.

error is simplified to error?: boolean as discussed.

coveralls · 2016-10-28T18:19:59Z

Coverage remained the same at 100.0% when pulling 8cc32e5 on unional:master into 08fdb1b on acdlite:master.

JaKXz · 2016-11-02T18:35:04Z

Sorry @unional for the delay - I'm still on the fence about the index.d.ts file being in the src directory and then having to be copied over in the babel script. I'd rather it be in the root of the project and just published alongside the lib folder unless that's against the TS standard?

unional · 2016-11-02T20:59:30Z

No problem. It is a convention that the .d.ts file lives along the .js file.
That's what tsc will produce if you wrote the library in TypeScript (that it transpiles the files to lib/dist).

I can move it to root if you still want that.

JaKXz · 2016-11-18T07:57:34Z

@unional thanks so much for your patience! I've had some offline conversations about this and it LGTM - I'll update the README and publish v1.1.0 soon.

unional · 2016-11-18T22:03:22Z

I have some update to the typings that can make it more useful.
Will submit another PR. Can you wait for that? Should be ready in an hour or so.

jayphelps mentioned this pull request Aug 8, 2016

Request to move ActionsObservable into a separate module redux-observable/redux-observable#95

Closed

JaKXz assigned acdlite Oct 27, 2016

JaKXz reviewed Oct 27, 2016

View reviewed changes

unional added 2 commits October 26, 2016 22:39

Create index.d.ts

bc4d9f8

Clean up index. Update package.json

cd45da8

unional force-pushed the master branch from a7b33bb to cd45da8 Compare October 27, 2016 05:42

Move index.d.ts

26a7bf3

JaKXz reviewed Oct 27, 2016

View reviewed changes

unional added 3 commits October 28, 2016 11:14

Copy typings to lib

3c83542

Simplify error

a542574

Update comment

8cc32e5

JaKXz merged commit 0e2d22b into redux-utilities:master Nov 18, 2016

unional mentioned this pull request Nov 18, 2016

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typings to the repo #33

Add typings to the repo #33

unional commented Jun 28, 2016

jayphelps commented Sep 9, 2016

JaKXz left a comment

coveralls commented Oct 27, 2016 •

edited

Loading

unional commented Oct 27, 2016 •

edited

Loading

coveralls commented Oct 27, 2016

coveralls commented Oct 27, 2016

coveralls commented Oct 27, 2016

JaKXz Oct 27, 2016

unional Oct 27, 2016

JaKXz Oct 27, 2016 •

edited

Loading

unional Oct 27, 2016

JaKXz Oct 27, 2016

JaKXz Oct 27, 2016 •

edited

Loading

JaKXz Oct 28, 2016

yangmillstheory Oct 28, 2016

unional Oct 28, 2016

yangmillstheory Oct 28, 2016 •

edited

Loading

unional Oct 28, 2016

yangmillstheory Oct 28, 2016

unional commented Oct 28, 2016

coveralls commented Oct 28, 2016 •

edited

Loading

JaKXz commented Nov 2, 2016

unional commented Nov 2, 2016

JaKXz commented Nov 18, 2016

unional commented Nov 18, 2016

Add typings to the repo #33

Add typings to the repo #33

Conversation

unional commented Jun 28, 2016

jayphelps commented Sep 9, 2016

JaKXz left a comment

Choose a reason for hiding this comment

coveralls commented Oct 27, 2016 • edited Loading

unional commented Oct 27, 2016 • edited Loading

coveralls commented Oct 27, 2016

coveralls commented Oct 27, 2016

coveralls commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaKXz Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaKXz Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangmillstheory Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unional commented Oct 28, 2016

coveralls commented Oct 28, 2016 • edited Loading

JaKXz commented Nov 2, 2016

unional commented Nov 2, 2016

JaKXz commented Nov 18, 2016

unional commented Nov 18, 2016

coveralls commented Oct 27, 2016 •

edited

Loading

unional commented Oct 27, 2016 •

edited

Loading

JaKXz Oct 27, 2016 •

edited

Loading

JaKXz Oct 27, 2016 •

edited

Loading

yangmillstheory Oct 28, 2016 •

edited

Loading

coveralls commented Oct 28, 2016 •

edited

Loading