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

JavaScript based REST interface client #1209

Merged
merged 8 commits into from Sep 13, 2015

Conversation

Projects
None yet
4 participants
@s-ludwig
Member

s-ludwig commented Aug 3, 2015

No description provided.

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Aug 3, 2015

Contributor
Contributor

Geod24 commented Aug 3, 2015

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 4, 2015

Member

I have actually started to work on the duplication issue (which is already present even without this addition), but the current RestInterface struct intended for this is still not ready for CT generation, because it currently requires the RestInterfaceSettings for a lot of things. I'll work on that later.

I'll probably create a 0.7.24 branch during the next couple of days and start merging your remaining changes in vibe.web.rest. Then I'd do a larger refactoring, moving things out of the vibe.web.rest module and switching over to use RestInterface there, too.

Member

s-ludwig commented Aug 4, 2015

I have actually started to work on the duplication issue (which is already present even without this addition), but the current RestInterface struct intended for this is still not ready for CT generation, because it currently requires the RestInterfaceSettings for a lot of things. I'll work on that later.

I'll probably create a 0.7.24 branch during the next couple of days and start merging your remaining changes in vibe.web.rest. Then I'd do a larger refactoring, moving things out of the vibe.web.rest module and switching over to use RestInterface there, too.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 5, 2015

Member

Cleaned up the commit history for review.

Member

s-ludwig commented Aug 5, 2015

Cleaned up the commit history for review.

header, // req.header[]
attributed, // @before
internal // req.params[]
}

This comment has been minimized.

@Geod24

Geod24 Aug 6, 2015

Contributor

IIUC, internal is for path attribute, isn't it ? Maybe that'll be a more descriptive name. Also body parameters are not meant to stay JSON forever. On that topic: any way to check the server setting ? It's not a big deal, but if someone disable parseQueryString for example, there's no way to detect it.

@Geod24

Geod24 Aug 6, 2015

Contributor

IIUC, internal is for path attribute, isn't it ? Maybe that'll be a more descriptive name. Also body parameters are not meant to stay JSON forever. On that topic: any way to check the server setting ? It's not a big deal, but if someone disable parseQueryString for example, there's no way to detect it.

This comment has been minimized.

@s-ludwig

s-ludwig Aug 6, 2015

Member

It's for path attributes or any other attributes that are passed using req.params. Since there are valid uses outside of path placeholders, I decided against using that as the name. With the "JSON" comment I just wanted to describe the current state, since I'm not sure yet how to best handle multiple formats in terms of the internal architecture.

Currently there is no way to check the server settings. Really the only possibility that I see would be to throw an exception from inside HTTPServerRequest if the .json field is accessed without the parseJson setting. Another, probably better, path would be to switch the whole JSON body parsing to happen lazily and optionally strongly typed.

@s-ludwig

s-ludwig Aug 6, 2015

Member

It's for path attributes or any other attributes that are passed using req.params. Since there are valid uses outside of path placeholders, I decided against using that as the name. With the "JSON" comment I just wanted to describe the current state, since I'm not sure yet how to best handle multiple formats in terms of the internal architecture.

Currently there is no way to check the server settings. Really the only possibility that I see would be to throw an exception from inside HTTPServerRequest if the .json field is accessed without the parseJson setting. Another, probably better, path would be to switch the whole JSON body parsing to happen lazily and optionally strongly typed.

@@ -0,0 +1,106 @@
module vibe.web.internal.rest.jsclient;

This comment has been minimized.

@Geod24

Geod24 Aug 6, 2015

Contributor

It could really use some documentation / documented unittest :)

@Geod24

Geod24 Aug 6, 2015

Contributor

It could really use some documentation / documented unittest :)

This comment has been minimized.

@s-ludwig

s-ludwig Aug 6, 2015

Member

I'll add an example in vibe.web.rest and the missing module header, as well as a doc string for generateInterface here.

@s-ludwig

s-ludwig Aug 6, 2015

Member

I'll add an example in vibe.web.rest and the missing module header, as well as a doc string for generateInterface here.

"vibe-d": { "path": "../../" }
},
"versions": ["VibeDefaultMain"]
}

This comment has been minimized.

@Geod24

Geod24 Aug 6, 2015

Contributor

Nitpick: Missing newline.

As per the example that follow, there's no comment so it's a bit hard on newcomers to see how useful it is.

@Geod24

Geod24 Aug 6, 2015

Contributor

Nitpick: Missing newline.

As per the example that follow, there's no comment so it's a bit hard on newcomers to see how useful it is.

This comment has been minimized.

@s-ludwig

s-ludwig Aug 6, 2015

Member

Will fix.

@s-ludwig

s-ludwig Aug 6, 2015

Member

Will fix.

Show outdated Hide outdated source/vibe/web/rest.d
Show outdated Hide outdated source/vibe/web/rest.d
Show outdated Hide outdated source/vibe/web/rest.d
@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Aug 6, 2015

Contributor

Hum, another P.R. blocked by 2.065 :(

Also, it looks like there's a discrepancy between the client and server with the refactoring:

REST route: GET /root/get_check/:param/:param2 []
add route GET /root/get_check/:param/:param2
[...]
no route match: GET /root/getCheck/one/two
No response written for /root/getCheck/one/two
REST call: GET http://127.0.0.1:8000/root/getCheck/one/two -> 404, 404

For the rest, the diff is large, so I'll probably have to try it out on my machine to be sure, but I like where it's heading at. I'm not that familiar with JS, but the new functionality looks cool 👍

Contributor

Geod24 commented Aug 6, 2015

Hum, another P.R. blocked by 2.065 :(

Also, it looks like there's a discrepancy between the client and server with the refactoring:

REST route: GET /root/get_check/:param/:param2 []
add route GET /root/get_check/:param/:param2
[...]
no route match: GET /root/getCheck/one/two
No response written for /root/getCheck/one/two
REST call: GET http://127.0.0.1:8000/root/getCheck/one/two -> 404, 404

For the rest, the diff is large, so I'll probably have to try it out on my machine to be sure, but I like where it's heading at. I'm not that familiar with JS, but the new functionality looks cool 👍

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 7, 2015

Member

Fixed the inconsistency (c5d3208) and 2.065 compilation error (ed739a9).

Member

s-ludwig commented Aug 7, 2015

Fixed the inconsistency (c5d3208) and 2.065 compilation error (ed739a9).

@s-ludwig s-ludwig changed the title from [WIP] JavaScript based REST interface client to JavaScript based REST interface client Aug 9, 2015

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 11, 2015

Member

BTW, for xref purposes, RestInterface also makes things like #662 and #1070 an easy target.

Member

s-ludwig commented Aug 11, 2015

BTW, for xref purposes, RestInterface also makes things like #662 and #1070 an easy target.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 11, 2015

Member

Rebased on master. I've also fixed the handling of optional parameters. Commit ca3def1 needs some re-review, because during rebase it was just a single big conflict with the write-to-headers change. Apart from that, everything except the default service URL comment and the possible merge of ParameterKind and WebParamAttribute.Origin should be addressed.

I'll have a look at the latter now. Regarding the URL, I don't remember exactly anymore, but I think I got some internal assertions due to an empty service URL, which was quite unobvious for the user. We could alternatively use something like "http://localhost/" or just add high-level assertions with a proper error message. The latter sounds like the better way to go...

Member

s-ludwig commented Aug 11, 2015

Rebased on master. I've also fixed the handling of optional parameters. Commit ca3def1 needs some re-review, because during rebase it was just a single big conflict with the write-to-headers change. Apart from that, everything except the default service URL comment and the possible merge of ParameterKind and WebParamAttribute.Origin should be addressed.

I'll have a look at the latter now. Regarding the URL, I don't remember exactly anymore, but I think I got some internal assertions due to an empty service URL, which was quite unobvious for the user. We could alternatively use something like "http://localhost/" or just add high-level assertions with a proper error message. The latter sounds like the better way to go...

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 11, 2015

Member

@Geod24, did you see any remaining issues? Otherwise I'd rebase/squash and merge now.

Member

s-ludwig commented Sep 11, 2015

@Geod24, did you see any remaining issues? Otherwise I'd rebase/squash and merge now.

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Sep 11, 2015

Contributor

I'll do a last sweep this evening ;)

Contributor

Geod24 commented Sep 11, 2015

I'll do a last sweep this evening ;)

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Sep 11, 2015

Contributor

Can't see anything.
All your project build out of the box with the new version ?

Contributor

Geod24 commented Sep 11, 2015

Can't see anything.
All your project build out of the box with the new version ?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 12, 2015

Member

I haven't tested all of them yet, but I'll do that now.

Member

s-ludwig commented Sep 12, 2015

I haven't tested all of them yet, but I'll do that now.

s-ludwig added a commit that referenced this pull request Sep 13, 2015

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 13, 2015

Member

Okay, when testing my projects I found one bug in the REST client (09ee6a2), but none in the new code. Will merge once Travis goes green.

Member

s-ludwig commented Sep 13, 2015

Okay, when testing my projects I found one bug in the REST client (09ee6a2), but none in the new code. Will merge once Travis goes green.

s-ludwig added a commit that referenced this pull request Sep 13, 2015

Merge pull request #1209 from rejectedsoftware/restjsclient
JavaScript based REST interface client

@s-ludwig s-ludwig merged commit 897521b into master Sep 13, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 13, 2015

Member

And thanks for reviewing this lengthy change set!

Member

s-ludwig commented Sep 13, 2015

And thanks for reviewing this lengthy change set!

@s-ludwig s-ludwig deleted the restjsclient branch Sep 13, 2015

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 13, 2015

Contributor

I would have loved to review this but got caught up in the busiest time of the past few years. Sorry!

Contributor

etcimon commented Sep 13, 2015

I would have loved to review this but got caught up in the busiest time of the past few years. Sorry!

// XHR setup
output.put(" var xhr = new XMLHttpRequest();\n");
output.formattedWrite(" xhr.open('%s', url, true);\n", route.method.to!string.toUpper);
static if (!is(RT == void)) {

This comment has been minimized.

@ColdenCullen

ColdenCullen Oct 14, 2015

Contributor

Sorry for not catching this sooner, but is RT actually defined here? I think it's causing this block to execute regardless of return type.

@ColdenCullen

ColdenCullen Oct 14, 2015

Contributor

Sorry for not catching this sooner, but is RT actually defined here? I think it's causing this block to execute regardless of return type.

This comment has been minimized.

@s-ludwig

s-ludwig Oct 14, 2015

Member

Thanks, looks like that... is() and __traits(compiles) are really suboptimal constructs w.r.t. correctness. I'll add a test and fix this.

@s-ludwig

s-ludwig Oct 14, 2015

Member

Thanks, looks like that... is() and __traits(compiles) are really suboptimal constructs w.r.t. correctness. I'll add a test and fix this.

This comment has been minimized.

@s-ludwig

s-ludwig Oct 14, 2015

Member

Err.. or rather just merge your PR and add a test ;)

@s-ludwig

s-ludwig Oct 14, 2015

Member

Err.. or rather just merge your PR and add a test ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment