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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial promise implementation #10

Closed
wants to merge 4 commits into from

Conversation

@lightswitch05
Copy link
Contributor

lightswitch05 commented Oct 22, 2015

In issue #9 - it was mentioned that pull requests adding promises would be welcomed.

For this implementation I decided to use the Q library for promises since it is a well known and feature-rich library. Lots of testing would be required since this was a quick change. Comments are welcomed 馃槂

@zeropaper

This comment has been minimized.

Copy link
Contributor

zeropaper commented Oct 23, 2015

Looks promising. Thanks!

We will review it within the next days and keep you in touch.

@lightswitch05

This comment has been minimized.

Copy link
Contributor Author

lightswitch05 commented Oct 23, 2015

馃槓馃槒馃槃馃槅馃槅馃槀馃憦馃憦馃憦

@zeropaper

This comment has been minimized.

Copy link
Contributor

zeropaper commented Oct 26, 2015

Some quick feedback.

I applied the patch and tried to run our e2e tests (which also rely on the SDK) but unfortunately it looks like the changes break the API.

My guess is that it has to do with the "callback as last argument" pattern and some functions working with variable amount of arguments. But it is only a guess.

Somebody else will do a more in-depth review next week and we will keep you updated.

@lightswitch05

This comment has been minimized.

Copy link
Contributor Author

lightswitch05 commented Oct 27, 2015

Thanks for the update. I am not surprised to hear there are problems. The unit tests are minimal and my own production usage is minimal as well, so it was hard to get decent code coverage. Hopefully the review shows minor bugs as I think promises would be a fantastic addition to this library.

@SebastianStamm

This comment has been minimized.

Copy link
Contributor

SebastianStamm commented Nov 6, 2015

I investigated the problems we are having when integrating the changes into our environment.

You are relying on the return of a promise from the HttpClient (https://github.com/lightswitch05/camunda-bpm-sdk-js/blob/feature/promises/lib/api-client/abstract-client-resource.js#L155). You added this promise implementation to the http-client that is shipped with the SDK.

In our applications however, we use a custom angular http client, that does not return a promise (https://github.com/camunda/camunda-commons-ui/blob/master/lib/services/HttpClient.js). Other consumers of the SDK might have similar setups.

Is it possible to implement the promise behavior in the SDK without relying on the underlying HttpClient providing them?

@lightswitch05

This comment has been minimized.

Copy link
Contributor Author

lightswitch05 commented Nov 6, 2015

Thanks for update.

Is it possible to implement the promise behavior in the SDK without relying on the underlying HttpClient providing them?

  • It would be possible, but the change would require a lot more time then I have.

In our applications however, we use a custom angular http client, that does not return a promise.

  • From a quick look, it seems like its just a wrapper and not a custom implementation. You could add a return in there to make it compatible. :)

Other consumers of the SDK might have similar setups.

  • The way the code currently is, it would be a breaking change.

While adding promise support directly to the SDK would be a major undertaking, it should be possible for the SDK to pass promises through if they are supplied - but to not rely on them. In my changes I optimized CamSDK.AbstractClientResource.list to be asynchronous - but it required the use of promises. I believe if those changes were reverted - support for promises could be added without relying on them. It would be nice if asynchronous behavior could be retained - I'm not sure how that would look with callbacks.

This should preserve backwards compatibility with other HttpClient implementations.
@lightswitch05

This comment has been minimized.

Copy link
Contributor Author

lightswitch05 commented Nov 6, 2015

Updated the branch to remove assumptions that HttpClient returns a promise. I was able to preserve the asynchronous behavior of CamSDK.AbstractClientResource.list.

@SebastianStamm

This comment has been minimized.

Copy link
Contributor

SebastianStamm commented Nov 9, 2015

It looks like it does not have any breaking changes anymore and I was able to run the camunda webapps with the change - awesome!

We should have some automated tests for promises inside the SDK itself. Can you provide the test-cases?

@lightswitch05

This comment has been minimized.

Copy link
Contributor Author

lightswitch05 commented Nov 9, 2015

I can look at it, but it doesn't look like I'm not going to have much time to spend on open source for the next couple weeks. Help would be appreciated

@SebastianStamm

This comment has been minimized.

Copy link
Contributor

SebastianStamm commented Nov 10, 2015

That's fine. We can help with the testing, too, but it's probably not happening this month as we are quite busy with the release at the end of November.

@lightswitch05

This comment has been minimized.

Copy link
Contributor Author

lightswitch05 commented Nov 17, 2015

Added some returns I had missed. Also added some karma and mocha tests involving promises.

SebastianStamm added a commit that referenced this pull request Dec 1, 2015
@SebastianStamm

This comment has been minimized.

Copy link
Contributor

SebastianStamm commented Dec 1, 2015

Alright, I just merged this PR with master.

Also I must say that I enjoyed working on this PR. The only thing left to do was to fix a small bug in one of the Task resources and fix the commit messages 馃憤

@lightswitch05

This comment has been minimized.

Copy link
Contributor Author

lightswitch05 commented Dec 1, 2015

Thanks for merging - glad to hear it went smoothly 馃帀 馃嵒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.