redux-utilities/flux-standard-actionPublic

NotificationsYou must be signed in to change notification settings
Fork142
Star4.7k

Add typings to the repo#33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

JaKXz merged 6 commits intoredux-utilities:masterfromunional:master

Nov 18, 2016

Merged

Add typings to the repo#33

JaKXz merged 6 commits intoredux-utilities:masterfromunional:master

Nov 18, 2016

Conversation

Copy link

Contributor

unional commentedJun 28, 2016

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

Thanks. 🌷

jayphelps mentioned this pull request

Aug 8, 2016

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

Closed

Copy link

jayphelps commentedSep 9, 2016

bump@acdlite

JaKXz assignedacdlite

Oct 27, 2016

JaKXz reviewed

Oct 27, 2016

View reviewed changes

Copy link

Contributor

JaKXz left a comment

Choose a reason for hiding this comment

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

@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.

unional added2 commits

October 26, 2016 22:39

Create index.d.ts

bc4d9f8

Clean up index. Update package.json

cd45da8

unional force-pushed themaster branch froma7b33bb tocd45da8Compare

October 27, 2016 05:42

Copy link

coveralls commentedOct 27, 2016•
edited
Loading

Coverage remained the same at 100.0% when pullingcd45da8 on unional:master into08fdb1b on acdlite:master.

Move index.d.ts

26a7bf3

Copy link

ContributorAuthor

unional commentedOct 27, 2016•
edited
Loading

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.

Copy link

coveralls commentedOct 27, 2016

Coverage remained the same at 100.0% when pulling26a7bf3 on unional:master into08fdb1b on acdlite:master.

2 similar comments

Copy link

coveralls commentedOct 27, 2016

Coverage remained the same at 100.0% when pulling26a7bf3 on unional:master into08fdb1b on acdlite:master.

Copy link

coveralls commentedOct 27, 2016

Coverage remained the same at 100.0% when pulling26a7bf3 on unional:master into08fdb1b on acdlite:master.

JaKXz reviewed

Oct 27, 2016

View reviewed changes

package.json

		"main":"lib/index.js",
		"typings":"src/index.d.ts",
		"files": [
		"lib"

Copy link

Contributor

Choose a reason for hiding this comment

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

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

src/index.d.ts Outdated

		* 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.

Copy link

Contributor

JaKXzOct 27, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

Contributor

Choose a reason for hiding this comment

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

Good point - it should be updated.

Copy link

Contributor

JaKXzOct 27, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link

Contributor

Choose a reason for hiding this comment

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

cc@yangmillstheory for your thoughts?

Copy link

Choose a reason for hiding this comment

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

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 beboolean, since no other value is meaningful?

Copy link

ContributorAuthor

unionalOct 28, 2016

Choose a reason for hiding this comment

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

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

Copy link

yangmillstheoryOct 28, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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 aretrue,false,undefined, ornull.

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

interfaceFoo{foo?:boolean}constfoo:Foo={foo:null}// no compiler errors

Copy link

ContributorAuthor

unionalOct 28, 2016

Choose a reason for hiding this comment

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

When user enablesstrictNullCheck, it is excluded from?.

Copy link

yangmillstheoryOct 28, 2016

Choose a reason for hiding this comment

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

Is there a way to disambiguate betweennull andundefined in code? I think 2.0 hasNull andUndefined.

unional added3 commits

October 28, 2016 11:14

Copy typings to lib

3c83542

Simplifyerror

a542574

Update comment

8cc32e5

Copy link

ContributorAuthor

unional commentedOct 28, 2016

@JaKXz I updatepackage.json to instructbabel to copy the file over tolib, so the editor can recognize the typings and it is still distributed underlib.

error is simplified toerror?: boolean as discussed.

Copy link

coveralls commentedOct 28, 2016•
edited
Loading

Coverage remained the same at 100.0% when pulling8cc32e5 on unional:master into08fdb1b on acdlite:master.

Copy link

Contributor

JaKXz commentedNov 2, 2016

Sorry@unional for the delay - I'm still on the fence about theindex.d.ts file being in thesrc 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 thelib folder unless that's against the TS standard?

Copy link

ContributorAuthor

unional commentedNov 2, 2016

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

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

Copy link

Contributor

JaKXz commentedNov 18, 2016

@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.

JaKXz merged commit0e2d22b intoredux-utilities:master

Nov 18, 2016

Copy link

ContributorAuthor

unional commentedNov 18, 2016

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.

unional mentioned this pull request

Nov 18, 2016

Delete flux-standard-action.jsontypings/registry#910

Open

Labels

None yet

6 participants

Movatterモバイル変換

Add typings to the repo#33

Add typings to the repo#33

Uh oh!

Conversation

unional commentedJun 28, 2016

Uh oh!

jayphelps commentedSep 9, 2016

Uh oh!

JaKXz left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commentedOct 27, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

unional commentedOct 27, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

coveralls commentedOct 27, 2016

Uh oh!

coveralls commentedOct 27, 2016

Uh oh!

coveralls commentedOct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaKXzOct 27, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaKXzOct 27, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangmillstheoryOct 28, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unional commentedOct 28, 2016

Uh oh!

coveralls commentedOct 28, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

JaKXz commentedNov 2, 2016

Uh oh!

unional commentedNov 2, 2016

Uh oh!

JaKXz commentedNov 18, 2016

Uh oh!

unional commentedNov 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coveralls commentedOct 27, 2016•
edited
Loading

unional commentedOct 27, 2016•
edited
Loading

JaKXzOct 27, 2016•
edited
Loading

JaKXzOct 27, 2016•
edited
Loading

yangmillstheoryOct 28, 2016•
edited
Loading

coveralls commentedOct 28, 2016•
edited
Loading