Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Improve typings#45

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

Merged
JaKXz merged 14 commits intoredux-utilities:masterfromunional:master
Jan 24, 2017
Merged

Improve typings#45

JaKXz merged 14 commits intoredux-utilities:masterfromunional:master
Jan 24, 2017

Conversation

@unional
Copy link
Contributor

@unionalunional commentedNov 18, 2016•
edited
Loading

I have added a few things:

  • Add type guard
  • Add generic types to the interface. The main usage is onisFSA andisError
  • Add test

This allows users to specify the type of thepayload andmeta easily.
Seetypings-test.ts for examples.

In the future, I would like to make theMeta type optional.
But need to wait formicrosoft/TypeScript#2175

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

@coveralls
Copy link

coveralls commentedNov 18, 2016•
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling421cac2 on unional:master intod9b70df on acdlite:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling133fbdf on unional:master intod9b70df on acdlite:master.

1 similar comment
@coveralls
Copy link

coveralls commentedNov 18, 2016•
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling133fbdf on unional:master intod9b70df on acdlite:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling5633fff on unional:master intod9b70df on acdlite:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling5633fff on unional:master intod9b70df on acdlite:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling5633fff on unional:master intod9b70df on acdlite:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pullingf542e79 on unional:master intod9b70df on acdlite:master.

1 similar comment
@coveralls
Copy link

coveralls commentedNov 18, 2016•
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pullingf542e79 on unional:master intod9b70df on acdlite:master.

Copy link
Contributor

@JaKXzJaKXz left a comment

Choose a reason for hiding this comment

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

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

package.json Outdated
"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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use theposttest lifecycle hook fortsc.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Nice, never use posttest before. :)

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

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Ok.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling31f0e76 on unional:master intod9b70df on acdlite:master.

1 similar comment
@coveralls
Copy link

coveralls commentedNov 19, 2016•
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling31f0e76 on unional:master intod9b70df on acdlite:master.

@unional
Copy link
ContributorAuthor

Updated based on feedback. 🌷

@coveralls
Copy link

coveralls commentedNov 19, 2016•
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling22229d3 on unional:master intod9b70df on acdlite:master.

@coveralls
Copy link

coveralls commentedNov 21, 2016•
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling78a9065 on unional:master intod9b70df on acdlite:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling78a9065 on unional:master intod9b70df on acdlite:master.

@unional
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

I think it is the right change. Whenpayload andmeta 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
Copy link
ContributorAuthor

cc/@tkqubo who add typings to DT.

tkqubo reacted with heart emoji

@coveralls
Copy link

coveralls commentedDec 11, 2016•
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pullingc9fa687 on unional:master into0c3752a on acdlite:master.

@unional
Copy link
ContributorAuthor

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

Copy link
Contributor

@JaKXzJaKXz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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

I have a couple more notes:

  • please runyarn to updateyarn.lock
  • theposttest action that runstsc will return an exit code of 1 if there is a problem with the typings, correct?

There are some more nitpicks I could make intypings-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
Copy link
ContributorAuthor

@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 usetslint (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 isposttest, I don't think it will stop the build.

please run yarn to update yarn.lock

I personally do not recommend checking inyarn.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
Copy link

coveralls commentedJan 9, 2017•
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling5216925 on unional:master into0c3752a on acdlite:master.

@unional
Copy link
ContributorAuthor

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

Copy link
Contributor

@JaKXzJaKXz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

@unional re:yarn.lock - as a maintainer it has actually made my lifemuch easier and helped me catch a number of issues regarding deps before merging PRs. I think the purpose of theyarn.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 agit bisect. I am going to ask that you update theyarn.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 ahoundCI 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 thetsc 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 ifposttest fails, any commands that depend ontest will not execute.

@JaKXzJaKXz self-assigned thisJan 10, 2017
@JaKXz
Copy link
Contributor

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pullingb9d81be on unional:master into00c2b9b on acdlite:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pullingb9d81be on unional:master into00c2b9b on acdlite:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pullingb9d81be on unional:master into00c2b9b on acdlite:master.

@unional
Copy link
ContributorAuthor

unional commentedJan 24, 2017•
edited
Loading

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

It does exist with non-zero. I was just concerning thatposttest 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 onmoduleA@1.2.3 and unfortunatelymoduleA@1.2.4 breaks your code, you won't know it until some users use your module and discover it is broken.

This is because during consumption, theyarn.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 usingyarn.lock withgreenkeeper would work out just fine?

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

package.json Outdated
"lodash.isstring":"^4.0.1",
"lodash.issymbol":"^4.0.1"
"lodash.issymbol":"^4.0.1",
"typescript-eslint-parser":"^1.0.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think all typescript dependencies should be indevDeps.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, of course. 😛

@JaKXz
Copy link
Contributor

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

@JaKXzJaKXz requested a review fromacdliteJanuary 24, 2017 00:57
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling73529c4 on unional:master into00c2b9b on acdlite:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling73529c4 on unional:master into00c2b9b on acdlite:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling73529c4 on unional:master into00c2b9b on acdlite:master.

package.json Outdated
"typescript-eslint-parser":"^1.0.2"
},
"dependencies": {
"eslint-plugin-typescript":"^0.1.0",
Copy link
Contributor

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

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling4157c98 on unional:master into00c2b9b on acdlite:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling4157c98 on unional:master into00c2b9b on acdlite:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling4157c98 on unional:master into00c2b9b on acdlite:master.

@JaKXzJaKXz merged commit6a2898a intoredux-utilities:masterJan 24, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@acdliteacdliteAwaiting requested review from acdlite

1 more reviewer

@JaKXzJaKXzJaKXz requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JaKXzJaKXz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@unional@coveralls@JaKXz
[8]ページ先頭
©2009-2025 Movatter.jp