Skip to content

Use @types instead of typings #106

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

Merged
merged 9 commits into from
May 22, 2018
Merged

Conversation

cprecioso
Copy link
Contributor

Closes #97

This PR updates the project to use typings taken from the @types packages in the npm registry. Everything is fairly straight-forward, except the methods package used for testing, that has no @types package, so I created a custom typing.

It also exports the typings directly through package.json, now that it does not rely on the typings tool to build them.

@coveralls
Copy link

coveralls commented Jun 19, 2017

Coverage Status

Coverage remained the same at 94.822% when pulling 9aee49c on cprecioso:@types into 7c9f89f on blakeembrey:master.

@blakeembrey
Copy link
Member

There's no rush to merge this as a breaking change - do you think we can get methods into DefinitelyTyped to replace the custom types also?

@blakeembrey
Copy link
Member

And thanks, by the way! It looks great 👍

@cprecioso
Copy link
Contributor Author

I made a PR for the methods typings in DT: DefinitelyTyped/DefinitelyTyped#17422

@cprecioso
Copy link
Contributor Author

do you think we can get methods into DefinitelyTyped to replace the custom types also?

Done!

@coveralls
Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage remained the same at 94.822% when pulling cfc0e1d on cprecioso:@types into 7c9f89f on blakeembrey:master.

package.json Outdated
"@types/concat-stream": "^1.6.0",
"@types/form-data": "0.0.33",
"@types/methods": "^1.1.0",
"@types/node": "^8.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Can we move @types/node to dev dependencies? Installing node for people using the browser could cause issues.

@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage remained the same at 94.822% when pulling 2f16a2b on cprecioso:@types into 7c9f89f on blakeembrey:master.

@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage remained the same at 94.822% when pulling 4413c2c on cprecioso:@types into 7c9f89f on blakeembrey:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.822% when pulling 4413c2c on cprecioso:@types into 7c9f89f on blakeembrey:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.822% when pulling 4413c2c on cprecioso:@types into 7c9f89f on blakeembrey:master.

@blakeembrey
Copy link
Member

I wanted to merge this, but I also just realised that it does entirely break the browser use-case and it's impossible to resolve until TypeScript supports it. Why? Because form-data is exported from the main package and it relies on the node.js typings to work. If we could somehow make sure form-data is not exported (instead creating a generic interface that works for both), we might be able to workaround the issue.

@blakeembrey
Copy link
Member

/cc @unional with #97.

@unional
Copy link

unional commented Jun 27, 2017

Maybe internalize types/npm-form-data as custom-typings?

@blakeembrey
Copy link
Member

@unional The main issue is that we'd need to expose those types somehow, whether it's global or not. This works with Typings because I enabled browser resolution separate to main resolution (specifically for this use-case). Currently, I think the only work-around will be trying very hard to make sure only custom interfaces that implement both the browser and server side but expose neither specific features are exported. It's a fragile thing, but it might be the only way for now.

@unional
Copy link

unional commented Jun 27, 2017

Sorry, I misread types/npm-form-data I thought the browser.d.ts was to export it globally.

There was a time I tried to do some browser-spec stuff, but I hit a wall related to webpack.

It probably too much work to get a HCF of both interface... :)

Any news on the browser-spec support?

@blakeembrey
Copy link
Member

None. I wouldn't expect it to happen. I think I might take a stab at this interface some time next week (busy this week) unless someone else does it. That will be v10. Then v11 will be a much heavier refactor bring it up to par with https://github.com/serviejs/servie and exposing a lighter-weight browser polyfill when you don't need middleware (e.g. https://github.com/mulesoft/js-client-oauth2/tree/master/src/request).

@wtrocki
Copy link

wtrocki commented Feb 24, 2018

Any chance to get that merged?

@blakeembrey
Copy link
Member

I'm going to merge and release it as is, for anyone wanting this they can bump directly to 10.x. I'm going to release 11.x shortly after.

@blakeembrey blakeembrey merged commit 10d7bcd into serviejs:master May 22, 2018
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.

5 participants