Migrate to RxJS 5 #15
Conversation
|
Hi @nfantone, thanks very much for taking your time to submit this. Having used Seneca with your library previously, it is pretty cool having you integrate my library. Having had a quick look over the pull request you have made, you have covered a lot of issues that I have wanted to clean up. Due to the fact that this PR requires the bump of RxJS from 4x to 5x, I will have this form part of a rx-amqplib v1 release, which I want to have out by early next week. |
|
No need to rush this. Also, unit tests would be good to have for a major |
|
I agree fully with unit tests, and is already part of the plan. If you wish, another PR with unit tests would be greatly appreciated. |
| } catch (e) { | ||
| } // This prevents a race condition | ||
| this.cancel(tag) | ||
| .onErrorResumeNext(Observable.of(false)); |
There was a problem hiding this comment.
@SkippyZA Not very sure on this change, TBH. Your try/catch is synchronous while this isn't, so I'm not exactly confident it'll swallow exceptions. I couldn't reproduce the "race condition" you mention on the comment.
An alternative is just to return false on the catch block.
|
Agreed. I'd say testing the API surface would suffice, for now. Since this is a wrapper, you don't need to test inner |
| build/ | ||
| node_modules/ | ||
| ### Typescript ### | ||
| dist/ |
There was a problem hiding this comment.
This is cool, but can you clear up the rest. I believe a .gitignore should only care about artifacts from project itself. IDE config files, OS files etc should be handled by the users own global .gitignore file.
There was a problem hiding this comment.
Really? Sure, no problem. But I really wouldn't rely on users' global settings. I'd rather have some extra harmless lines on a text file than making a contributor change his PR because he involuntary pushed some .Trash-343dsf or ~RxChannel.ts file.
There was a problem hiding this comment.
It's your call, though! I don't mind removing them, if you think it's cleaner.
|
It seems there is a conflict with |
|
@SkippyZA You may want to put the release on hold for a moment. I've trying to push some things to make it easier to work with
|
|
Great work with the other PRs @nfantone Will merge this into the dev branch, but wont do a version bump yet. |
|
I'm very happy this PR exists. We've started using rxjs 5 and was just about to start my own fork but for now have pulled in nfantone's fork. If you need any help please let me know. Maybe I can spare some time and help get this PR merged or help in some other way. This is a great project. Thank you! |
|
I don't really know what's needed to merge this. @SkippyZA rebased the merge to another branch and it conflicted. Anyhow, I was working on updating things so as to avoid using |
|
Sorry gents, been slightly distracted with changing of jobs at the end of this month. I switched this to develop-v1 as I don't want to merge directly into master until we have 0.5.0 of amqplib in. I have also had the intention of building a basic set of test and release it as a v1 build. |
|
No worries, @SkippyZA. The thing with updating to I don't know what's the best solution here: update typings to use native EDIT EDIT 2 |
Hi, @SkippyZA!
I'm the lead maintainer on seneca-amqp-transport and while looking for a way to enhance that module I thought of including RxJS for its API. I started rolling something of my own, but then, shortly after, I stumbled upon your
amqplibwrapper and thought that it was just what I needed. So I built a little RPC module on top of it - but while doing so, I was depending on a local fork ofrx-amqplibwhich included some updates here and there. And here it is.This PR includes:
5.0.0-rc1..editorconfigandyarn.lockfiles.dist/andtypings/directories..gitignore.Our intention is to replace
amqplibwithrx-amqplibin our AMQP transport in the near future. We are currently using our internal fork for development and would be great to see at least some of these changes back merged to your library.I realize this may be a lot for a single PR (though, most of it is just config changes), but the intention was just to share what I've done. You can take only the bits that interest you. Or don't! Totally up to you.
Also, we have a Travis script for building sources and creating the now gone
dist/directory (same goes fortypings/). I didn't include it in the PR for obvious reasons. I see you plan on integrating CircleCI: you could accomplish the exact same thing we did using acircle.ymlfile or similar.Thanks for your work on this! Was glad to find your module on npm and having some reliable leverage that kept me from starting from scratch.