Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type definitions #6

Merged
merged 1 commit into from
Dec 29, 2017
Merged

Add type definitions #6

merged 1 commit into from
Dec 29, 2017

Conversation

demurgos
Copy link
Member

@demurgos demurgos commented Dec 28, 2017

This commit adds type definitions for plugin-error.
These types are adapted from the ones of gulp-util.
The main changes are that the types are more based on the
documentation instead of the implementation. It means that the error
property used internally is no longer exposed as an option (it matches
the tests). It also tries to prevent the use of the plugin option if
it will be overriden. It also adds support for inference of additional
properties from wrapped errors when the second argument is a real error.

This commit also adds some tests for the types. It just runs the
compiler with the noEmit option and checks for errors.

I had to set explicit versions for the test dependencies (instead of *)
because their latest versions require Node 4+ (the CI runs on Node 0.10, 0.12 and 3).

I tested it locally and as a replacement for gulp-util for the gulp-tslint plugin.

Closes #5

This commit adds type definitions for `plugin-error`.
These types are adapted from the ones of `gulp-util`.
The main changes are that the types are more based on the
documentation instead of the implementation. It means that the `error`
property used internally is not longer exposed as an option (it matches
the tests). It also tries to prevent the use of the `plugin` option if
it will be overriden. It also adds support for inference of additional
properties from wrapped errors when the second argument is a real error.

This commit also adds some tests for the types. It just runs the
compiler with the `noEmit` option and checks for errors.

Closes gulpjs#5
@demurgos
Copy link
Member Author

demurgos commented Dec 28, 2017

@phated
Could you take a look at it?
Among the most popular gulp packages relying on gulp-util, it seems that only gulp-tslint is blocked by the lack of types for plugin-error. It would also help gulp-typescript which currently has its own duplicated types hosted on its repo. The gulp-tslint author notified me that he is ready update its plugin so merging and publishing this PR would allow a fix for gulp-tslint.

@phated
Copy link
Member

phated commented Dec 29, 2017

Looks great! Thanks @demurgos - I'm going to merge this but not publish until I do some maintenance on the repo.

@phated phated merged commit ec8404a into gulpjs:master Dec 29, 2017
@demurgos
Copy link
Member Author

demurgos commented Jan 13, 2018

Could you publish the current version as a semver-minor update?

I know that you'd prefer to first do some maintenance, but it's better to not block publication if you don't have time to do it at the moment.
I could also try to help you with the maintenance if possible. I can't add integration with third-parties, but can update the README or apply some changes (code style?). I could try to replicate what you have on some reference repo (Vinyl for example?).

@phated
Copy link
Member

phated commented Jan 13, 2018

@demurgos I actually have the major version bump ready but was waiting on some licensing feedback. I didn't get a chance to finish it once I got the response but I can later today (when I get back to my other computer).

Side note: a semver-minor when below 1.0 has the same semantics of a major above 1.0 - so in this case, a semver minor is the same as bumping to 1.0

@demurgos
Copy link
Member Author

demurgos commented Jan 13, 2018

Ok, thanks for the update.
(I was proposing a semver minor to let 1.0.0 be the version with your changes, but yes a semver patch could have been better since there is no implementation change)

@phated
Copy link
Member

phated commented Jan 14, 2018

@demurgos published as 1.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants