-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Feat: Import TypeScript typings #1487
Conversation
- Import typings from DefinitelyTyped into repo. - Update typings header to cite contributors from history and git blame. - Update package.json to add typings field.
Thanks. I'll have a look at it. This evening (CET).
Nils
Am 19. Dezember 2018 00:22:46 MEZ schrieb Tim Lindvall <notifications@github.com>:
…- Import typings from DefinitelyTyped into repo.
- Update typings header to cite contributors from history and git
blame.
- Update package.json to add typings field.
Testing done:
- Verified typings against ***@***.***/syntax` by removing its dependency
on ***@***.***/handlebars` and ensuring VSCode recognized the new typings
from the package directly.
Open questions:
- If I'm reading contributing.md correctly, this should target the 4.x
branch in the upstream repo, correct? If not, let me know and I'll
either update this or open a new PR.
- Do we want to add linting to the typings file? If so, what type of TS
linting rules would be preferred?
Thanks for taking the time to review this PR! Let me know if you have
any questions or concerns.
You can view, comment on, or merge this pull request online at:
#1487
-- Commit Summary --
* Feat: Import TypeScript typings
-- File Changes --
A lib/handlebars.d.ts (356)
M package.json (1)
-- Patch Links --
https://github.com/wycats/handlebars.js/pull/1487.patch
https://github.com/wycats/handlebars.js/pull/1487.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#1487
--
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
|
awesome! |
Merged it, thanks. Should we notify DefinitelyTyped about this, so that changes and fixes to the file are made here in the future? Update: Please don't notify them yet. I have not made a release yet and I'm not going to do this today. Let's finish the linting discussion first and maybe wait until after christmas. I'm not going to have the time in the next couple of days. |
How many dependencies would we pull into the project by adding tslint?
The same linting rules that were applied in the DefinitelyTyped project. |
Thanks for merging and the feedback! Regarding the above items...
|
Is this a feature? Should we increase the minor version for this change? Probably, because your typescript build will break when you go back... |
Calling it a new feature and making a minor version bump sounds reasonable to me. 👍 |
I've just started removing the package from DefinitelyTyped, but I noticed that information will be lost. Despite what I said earlier, I think we should take over those things and include them into the normal build process. Otherwise, taking the types over is essentially a loss of quality, because they might diverge from the actual API. @zimmi88 Could you do that? |
@nknapp, sure I can work on this. I'll post a PR when it's ready. |
thanks |
@nknapp, finally have the bandwidth to move this testing and linting over. And finally getting the bandwidth to catch up on the latest posts to this repo. I'm really sorry... this PR appears to have caused a lot of headaches I didn't anticipate. =( Given the discussion in other threads, did you want to move forward with supporting types with Handlebars (and, in turn, I'll continue with porting over linting and tests), or did you want to roll back to the previous status quo - having other packages rely on |
I was having a rollback in mine, but it would have been a bad option, actually. It would have been another breaking change in a patch-release, so I am happy to say: Kudos to @mike-north and @chriskrycho from DefinitelyTyped for their help in making this happen. Mike seems to have removed the bugs from the ember-typings that prevented the merge. As far as I know, everything is working again. I haven't tested that myself though. I would appreciate if you continue porting the linter-rules and tests. |
The TL;DR of what was done to resolve these issues upstream
|
Thanks for these explanations. So, if anyone still uses |
Depends which 3.x release, but yes -- if and only if they have and older non-deprecated version |
Testing done:
@glimmer/syntax
by removing its dependency on@types/handlebars
and ensuring VSCode recognized the new typings from the package directly.Open questions:
Thanks for taking the time to review this PR! Let me know if you have any questions or concerns.