Remove extra initial (blank) message for streaming requests #37

gabrielgrant · 2017-06-28T14:34:15Z

Fixes #35

As mentioned in that issue, AFAICT there doesn't seem to be any reason for this code path to exist, and right now it is erroneously triggered every time, because args is an empty buffer that evaluates as "truthy"

paralin/grpc-bus#37

paralin · 2017-06-28T21:20:06Z

Hey, thanks for the PR.

Please squash your commits and follow the Angular Git Commit Guidelines when formatting your PR.

Upgrading typescript seems to have broken rxjs. Please make sure those dependencies are updated as well and the tests should pass.

You should be able to run npm run test and pass.

Thanks! ~Christian

paralin

Please follow the Angular Commit Style Guide and try to fix the breaking test: https://travis-ci.org/paralin/grpc-bus/builds/247978282?utm_source=github_status&utm_medium=notification

Thanks!

gabrielgrant · 2017-06-29T21:38:27Z

As I mentioned in #36 , I'm currently using the older (working) protobuf v5 version of grpc-bus (v1.0), so that's what I've branched off of for this patch. Unfortunately I'm not able to tackle the full protobuf v6 upgrade at the moment, so was hoping this could be released as a 1.0.1 (I created a pre-release for our purposes here: https://github.com/gabrielgrant/grpc-bus/releases/tag/v1.0.1-dev )

Because I'm branching off the older version, though, I think that bumping the dependency versions is going to result in a conflict when trying to merge into HEAD (since those deps been updated since v1.0). Not sure what the best way to avoid that is... maybe creating a v1.0 release branch to add patch releases to?

gabrielgrant · 2017-06-29T21:42:49Z

...I'm assuming you don't want to move the commits that broke master onto a separate protobuf-v6 branch, do you? From my POV that would be the best end result (master would be in a working state again), but I understand re-writing history on master of a public repo isn't exactly considered best-practice :/

paralin · 2017-06-29T22:09:06Z

Done, master is now pbjs 5. Please re-base your patch accordingly

gabrielgrant · 2017-08-01T22:53:39Z

Sorry for taking forever to get back to this. Got sidetracked on some other stuff, but should be getting back to this next week (when I'll hopefully also be tackling #40 )

paralin · 2017-08-01T22:54:49Z

No worries, I've almost completely abandoned this project myself just because I'm not actively using it right now

gabrielgrant · 2017-08-01T23:02:01Z

Yea, totally understand that it's not a priority for you, but really appreciate you keeping it up, as it still seems to be the best way to do bi-di streaming to/from a browser. We are actively using it (with a websocket client/server implementation on top), so happy to help with maintenance if you're interested.

paralin · 2017-08-01T23:20:03Z

Nice, the fact that you're using it means it works. We should try to work on the protobufjs 6 compatibility in the coming weeks... Not sure if we have to decide on a version, or if we can make it compatible with both.

Fixes paralin#35

set version to 2.4.2 (latest) since version 2.1.6 doesn't build: node_modules/@types/lodash/index.d.ts(9240,74): error TS2304: Cannot find name 'object'. node_modules/@types/lodash/index.d.ts(13129,29): error TS2304: Cannot find name 'object'. node_modules/@types/lodash/index.d.ts(20002,33): error TS2304: Cannot find name 'object'.

see ReactiveX/rxjs#2539 fixed in ReactiveX/rxjs#2722

gabrielgrant · 2017-08-16T21:49:50Z

The pushed commits should follow the style guide, and I've updated deps to fix build errors, but I'm still seeing errors with the tests:


/home/gabriel/repos/grpc-bus/src/server/server.ts[1, 18]: variable 'IGBClientMessage' used before declaration
/home/gabriel/repos/grpc-bus/src/server/server.ts[2, 16]: variable 'IGBCreateService' used before declaration
/home/gabriel/repos/grpc-bus/src/server/server.ts[4, 3]: variable 'IGBReleaseService' used before declaration
/home/gabriel/repos/grpc-bus/src/server/server.ts[5, 18]: variable 'IGBCreateCall' used before declaration
/home/gabriel/repos/grpc-bus/src/client/call.ts[2, 9]: variable 'IGBClientMessage' used before declaration
/home/gabriel/repos/grpc-bus/src/client/service.ts[10, 7]: variable 'ICallHandle' used before declaration
/home/gabriel/repos/grpc-bus/src/client/client.ts[2, 9]: variable 'IGBClientMessage' used before declaration

It's not clear to me what these particular variables/declarations have in common/are doing differently from other similar lines in the same files. Any idea what the issue(s) could be there?

paralin · 2017-08-16T21:56:30Z

Let me take a look

gabrielgrant · 2017-08-16T23:09:43Z

Cool, thanks a lot. I'm happy to actually make the fixes, but I don't have much of an idea of where to start debugging, and doing some digging on Google didn't really give me any clues.

And again, to be clear: these problems only crop up when running the npm run test -- npm run build completes successfully, and creates a working package (at least it seems to be fully working -- we're using it :P)

paralin

Just a few minor things:

Try to avoid capital letters and () in your commit message as per the general Angular commit style
When doable include a category: fix(package): pin typescript to fix lodash build error
fix(package): bump rjxs to 5.4.2 to fix ts2.4 build error

paralin · 2017-08-17T22:16:52Z

package.json

@@ -24,7 +24,7 @@
  },
  "dependencies": {
    "lodash": "^4.0.0",
-    "rxjs": "5.0.0-rc.5"
+    "rxjs": "5.4.2"


Can we pin this to something a bit more liberal, like rxjs at ~5.4.0?

The specific fix needed was in 5.4.2, so ~5.4.0 won't do, but I believe ~5.4.2 should work. I'll make the change.

OK, this change is good. ~5.4.2, that is.

paralin · 2017-08-17T22:17:09Z

package.json

@@ -41,7 +41,7 @@
    "protobufjs": "^5.0.0",
    "ts-node": "^1.7.0",
    "tslint": "^4.0.0",
-    "typescript": "^2.1.0"
+    "typescript": "2.4.2"


Again here, I'd prefer ^2.4.0 to 2.4.2 speciically

Although this is a compiler question so it's not actually that critical to do this, I suppose

Again, I believe there was a breaking change specifically between 2.4.0 and 2.4.1, so I don't think ^2.4.0 is a good idea, but ~2.4.2 should work. That being said, the fact that they introduced a breaking change in a patch release seems like perhaps a good reason to not trust that future patch releases will be problem-free, and given this is the compiler, I'd be a bit more inclined to leave this pegged exactly than with RXJS. I don't have super-strong feelings either way, though. What do you prefer?

If you want to keep it pegged that's fine. I see what you're saying about the breaking changes

paralin

Please also add a commit body to your commits, where you explain what you did briefly. You can end the message with a line that says something like

Fixes #37

But in this case you will want to be sure your commit messages have at least as much info as your PR messages, because the commit message is what lasts.

paralin · 2017-08-18T22:37:10Z

Npm will shrinkwrap the packages to lock them to a specific version. Then greenkeeper goes and sends a notification when they are updated but still within the range.

paralin · 2017-09-19T22:37:01Z

hey, apologies for not getting around to this for so long. I will keep an eye out for PRs / messages on this repo this week, and try to get it all merged and sorted for you. currently waiting for a revision of this PR, though.

gabrielgrant · 2017-09-20T00:42:31Z

Hey, no worries at all, and sorry I haven't gotten back to this -- we've been super busy getting ready for a release that should be happening tomorrow (fingers crossed!). In prep for that, we've made a few more updates on my release branch -- specifically, added support for GRPC metadata. Hopefully should have time to clean this all up and get it all properly PRed for you to merge into your master in the next week.

gabrielgrant force-pushed the fix-stream-request branch from d9cd341 to 797738a Compare June 28, 2017 15:07

gabrielgrant added a commit to gabrielgrant/grpc-bus-websocket-proxy-server that referenced this pull request Jun 28, 2017

upgrade grpc-bus to use a pre-release that fixes streaming request bug

2f121fe

paralin/grpc-bus#37

paralin requested changes Jun 28, 2017

View reviewed changes

paralin force-pushed the master branch from a58dcf8 to 7ac87a0 Compare June 29, 2017 22:04

gabrielgrant added 3 commits August 16, 2017 14:39

fix: Remove extra initial (blank) message for streaming requests

faf0329

Fixes paralin#35

fix: upgrade rxjs to 5.4.2 to fix compile error with TS 2.4

3006e76

see ReactiveX/rxjs#2539 fixed in ReactiveX/rxjs#2722

gabrielgrant force-pushed the fix-stream-request branch 2 times, most recently from 78b8b4b to 3006e76 Compare August 16, 2017 21:45

gabrielgrant mentioned this pull request Aug 16, 2017

Serialize error message #41

Open

paralin approved these changes Aug 17, 2017

View reviewed changes

paralin requested changes Aug 17, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove extra initial (blank) message for streaming requests #37

Remove extra initial (blank) message for streaming requests #37

gabrielgrant commented Jun 28, 2017

paralin commented Jun 28, 2017 •

edited

paralin left a comment

gabrielgrant commented Jun 29, 2017

gabrielgrant commented Jun 29, 2017 •

edited

paralin commented Jun 29, 2017

gabrielgrant commented Aug 1, 2017

paralin commented Aug 1, 2017

gabrielgrant commented Aug 1, 2017 •

edited

paralin commented Aug 1, 2017

gabrielgrant commented Aug 16, 2017

paralin commented Aug 16, 2017

gabrielgrant commented Aug 16, 2017 •

edited

paralin left a comment •

edited

paralin Aug 17, 2017

gabrielgrant Aug 18, 2017

paralin Sep 19, 2017

paralin Aug 17, 2017

paralin Aug 17, 2017

gabrielgrant Aug 18, 2017

paralin Sep 19, 2017

paralin left a comment

paralin commented Aug 18, 2017

paralin commented Sep 19, 2017

gabrielgrant commented Sep 20, 2017

Remove extra initial (blank) message for streaming requests #37

Are you sure you want to change the base?

Remove extra initial (blank) message for streaming requests #37

Conversation

gabrielgrant commented Jun 28, 2017

paralin commented Jun 28, 2017 • edited

paralin left a comment

Choose a reason for hiding this comment

gabrielgrant commented Jun 29, 2017

gabrielgrant commented Jun 29, 2017 • edited

paralin commented Jun 29, 2017

gabrielgrant commented Aug 1, 2017

paralin commented Aug 1, 2017

gabrielgrant commented Aug 1, 2017 • edited

paralin commented Aug 1, 2017

gabrielgrant commented Aug 16, 2017

paralin commented Aug 16, 2017

gabrielgrant commented Aug 16, 2017 • edited

paralin left a comment • edited

Choose a reason for hiding this comment

paralin Aug 17, 2017

Choose a reason for hiding this comment

gabrielgrant Aug 18, 2017

Choose a reason for hiding this comment

paralin Sep 19, 2017

Choose a reason for hiding this comment

paralin Aug 17, 2017

Choose a reason for hiding this comment

paralin Aug 17, 2017

Choose a reason for hiding this comment

gabrielgrant Aug 18, 2017

Choose a reason for hiding this comment

paralin Sep 19, 2017

Choose a reason for hiding this comment

paralin left a comment

Choose a reason for hiding this comment

paralin commented Aug 18, 2017

paralin commented Sep 19, 2017

gabrielgrant commented Sep 20, 2017

paralin commented Jun 28, 2017 •

edited

gabrielgrant commented Jun 29, 2017 •

edited

gabrielgrant commented Aug 1, 2017 •

edited

gabrielgrant commented Aug 16, 2017 •

edited

paralin left a comment •

edited