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

Removed Node transport from automatic detection #204

Closed
wants to merge 8 commits into from

Conversation

jonahbron
Copy link
Contributor

@jonahbron jonahbron commented Jun 8, 2018

This is an alternative to https://github.com/improbable-eng/grpc-web/pull/203/files, and arguably a better solution. The downside is that this is a breaking change for any Node users, which means merging this will require a major version increment. This change simply requires that the developer import the Node transport if they want it, instead of automatically detecting it. This ensures the Node transport can be omitted with static analysis.

/resolves #191

@jonahbron
Copy link
Contributor Author

jonahbron commented Jun 9, 2018

I've been trying to get all of the integration tests passing again, but it's not cooperating. I moved the same exact Nodejs detection into the unit test file, but it's not working as expected. The only job that fails because of my changes is the Node.js one. The others are just timeouts, I assume the test took too long.

https://travis-ci.org/improbable-eng/grpc-web/builds/389948600

Can anyone more familiar with the Travis config here offer input on how to get that test passing?

@johanbrandhorst
Copy link
Contributor

This looks like a worthwhile change to me, and thanks for taking the time to put it together @jonahbron. Unfortunately I'm not confident enough to say what's wrong exactly, so I've requested a review from @MarcusLongmuir.

Obviously, this being a breaking change, we may want to sit on it a bit. I'm not sure what the teams policy for v2.0 is at the moment.

@johanbrandhorst
Copy link
Contributor

Having thought about it, we're still in the 0.x range of semantic versioning so I think a breaking change could be alright (going to 0.7).

@easyCZ
Copy link
Contributor

easyCZ commented Sep 9, 2018

As part of this, we may want to consider adding support for setting of the default transport. This would eliminate the need to always explicitly override the transport in Node. On the other hand, it's easy to wrap by the client to auto-add the node-transport so I'm not too fussed.

@jonny-improbable
Copy link
Contributor

I believe this can now be closed as #265 is merged and will be released in 0.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Will require a major version increment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using grpc-web-client with Angular (angular-cli)
4 participants