-
Notifications
You must be signed in to change notification settings - Fork 234
Conversation
cc @edmorley any response. 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thank you for opening a PR :-)
@eliperelman this looks reasonable to me, and minimal impact + tested on Travis.
I can't say as to whether the typings are accurate (beyond the test) but I guess they've been working for people via https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/webpack-chain/index.d.ts so should be fine, right?
I'm on vacation from tomorrow until January, so if you're fine with this could you merge/release? :-)
@edmorley would you take a PR using TypeScript for this project? We do not need to turn strict rules on so it will feel like JavaScript so we do not need to fight the tool but we can take advantage of it. If you will maintain this file at the end, I see more value on using TypeScript. I take the take the responsibility to convert the project so you can focus on something else. |
@yordis We won't be using TypeScript in this project other than typings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you switch this to use 2-space indentation like the rest of the codebase? Thanks!
💔 <--- On reactions Any particular reason? You can turn all the TypeScript noise off and maintain the typespec as you are doing it today but in the code 💔 |
@yordis we don't use TypeScript, and maintaining tooling and syntax around a language we don't use increases our maintenance burden. |
Sorry for the delay here. Could you rebase on master and adjust the typings to account for #133? @eliperelman once those changes made, any other items you'd like to see addressed, or good to merge? |
I adjusted the type declaration file against the latest documentation. review need. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates - looks great now :-)
Released in webpack-chain v5.2.0: |
It was added in #132 but has always been unused.
It was added in #132 but has always been unused.
Ref DefinitelyTyped/DefinitelyTyped#27519
And inspire #123 (Don't know why this PR closed.
CC @edmorley