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

migrate from tsd to typings #382

Merged
merged 5 commits into from
Apr 11, 2016
Merged

migrate from tsd to typings #382

merged 5 commits into from
Apr 11, 2016

Conversation

trevj
Copy link
Contributor

@trevj trevj commented Apr 7, 2016

typings is the replacement for tsd:
https://github.com/typings/typings

My conclusion, having done this, is that typings is marginally less annoying than tsd. At least the config file no longer references Github commits.

Anyway, it's pretty much as before except I've opted for the "top-level" .d.ts rather than the individual .d.ts' as we've done to date, e.g.:

<reference path='../../../third_party/typings/browser.d.ts' />

I did this because it saves several lines and because I see mentions on typings' website of the directory layout within typings/ changing between releases.

freedomjs was the only typing that presented a challenge: the type of freedom differs depending on whether your code runs inside or outside of a freedomjs module. The freedom-core-env and freedom-module-env typings exist for this purpose (they're essentially one-liners and you should include one or the other, depending on the environment) - but if you want the convenience of using the top-level import then you're out of luck.

My solution was to specify only freedom in typings.json and have each freedom-using source file specify the type of freedom immediately after imports, e.g.:

declare var freedom: freedom.FreedomInModuleEnv;

Horrible? Maybe...I'm loathe to lose the convenience of that top-level import just for freedomjs' weirdness, though. egrep -ir 'declare (const|var) freedom' src/|wc -l tells me that 47/117 source files needed the declare - and I think that will fall, in time. Would appreciate your thoughts on this.


This change is Reviewable

@jpevarnek
Copy link
Contributor

This is fantastic, thanks for looking at it! Quick note that we talked in person about browser.d.ts, but 👍 at your discretion


Reviewed 93 of 93 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/arraybuffers/arraybuffers.ts, line 1 [r1] (raw file):
👍 🎆


src/integration-tests/socks-echo/base-spec.core-env.ts, line 34 [r1] (raw file):
Should we have a space before any?


Comments from Reviewable

@trevj
Copy link
Contributor Author

trevj commented Apr 8, 2016

Awesome, thanks for reviewing. I read through the docs again and it (still) seems like browser.d.ts is what we should be using.

Since this is a pretty majorly break change, I'll probably hold off on submitting until I get uproxy working well with it.


Review status: 90 of 93 files reviewed at latest revision, 2 unresolved discussions.


src/integration-tests/socks-echo/base-spec.core-env.ts, line 34 [r1] (raw file):
Yes. Done, and in slow.core-enc.spec.ts.


Comments from Reviewable

@jpevarnek
Copy link
Contributor

Sounds good, let me know how I can help.


Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@trevj
Copy link
Contributor Author

trevj commented Apr 11, 2016

Actually, seems like uProxy compiles just fine against this. Merging...

@trevj trevj merged commit d4acdb7 into master Apr 11, 2016
@trevj trevj deleted the trevj-typings branch April 14, 2016 19:45
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