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

feat(rest): Single-server RestApplication #803

Merged
merged 1 commit into from Dec 18, 2017
Merged

Conversation

kjdelisle
Copy link
Contributor

@kjdelisle kjdelisle commented Dec 14, 2017

Description

This implementation simplifies the use-case for anyone that wants
to create a single-server REST application without needing to
handle RestComponent.

Related issues

connected to #770
(see #770 (comment)) for details

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide

export const ERR_NO_MULTI_SERVER = format(
'RestApplication does not support multiple servers!',
'To create your own server bindings, please extend the Application class.',
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: looks like an extra , here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's the coding style to have trailing , after the last argument if multiple lines are used.

},
});
// tslint complains about unused vars if app isn't used.
app.findByTag('server');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious: if I don't call app.findByTag, will I create a rest app with multiple servers successfully? or I will receive an error? I feel the later one would be the expected behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @kjdelisle uses this line to work around a tslint style complaint. But as I commented, We should add /* tslint:disable-next-line:no-unused-variable */ to make an exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see! was confused with the comment, now it makes sense.

bar: RestServer,
},
});
// tslint complains about unused vars if app isn't used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either remove const app = or add /* tslint:disable-next-line:no-unused-variable */.

@raymondfeng
Copy link
Contributor

Please fix the commit log to use feat(rest) instead of feature(rest) to avoid build failure.

}

server(server: Constructor<Server>, name?: string): Binding {
if (this.findByTag('server').length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be this.findByTag('server').length > 1 to account for the server that comes with RestApplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of the tests is to demonstrate that all of those approaches are actually using the server function under the hood, which also includes the automatic instantiation and mounting of the RestServer as a part of RestComponent's loading.

We want this function to work when no servers are present because this function is exactly how we get our first instance. If the count check was > 1, it would mean that we could always get two servers (since 1 > 1 is false, it won't throw)

@kjdelisle
Copy link
Contributor Author

@raymondfeng Fixed the commit header and added the tslint exemptions. :)

@kjdelisle kjdelisle changed the title feature(rest): Single-server RestApplication feat(rest): Single-server RestApplication Dec 14, 2017
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nitpick. LGTM


it('when attempting bind multiple servers via RestComponent', () => {
class OtherRestComponent extends RestComponent {}
expect.throws(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no error message check for this test case?

Error,
ERR_NO_MULTI_SERVER,

// works.
async function startServerCheck(app: Application) {
const server = await app.getServer(RestServer);
server.handler((sequence, request, response) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we know that RestApplication has only a single server instance, I'd like helper methods like handler and sequence to be added to RestApplication.

To me, that's the most important part about RestApplication, because it brings us back to the easy-to-use design we had before the REST component was extracted from core.

const app = new RestApplication();
app.handler((sequence, request, response) => {
  sequence.send(response, 'hello world');
});

Since this is an additional change, I am ok to make it later, in a follow-up pull request.

@bajtos
Copy link
Member

bajtos commented Dec 15, 2017

@kjdelisle I think this should be connected to #661, in fact I think #661 can be closed once this is landed. Thoughts?

@kjdelisle
Copy link
Contributor Author

@bajtos I guess that depends on how much you dislike the UX with respect to multi-server. If we decide to implement multi-server, it'll still mean that devs will need to override the start function and retrieve their servers to configure them individually.

If we're not terribly concerned since it provides simplicity for the more common use case, then that's fine with me and we can close #661 when this lands.

@kjdelisle kjdelisle force-pushed the rest/application branch 2 times, most recently from bb54257 to 13b9712 Compare December 15, 2017 21:23
@kjdelisle
Copy link
Contributor Author

@slnode test please

This implementation simplifies the use-case for anyone that wants
to create a single-server REST application without needing to
handle RestComponent.
@kjdelisle kjdelisle merged commit 80638b4 into master Dec 18, 2017
@kjdelisle kjdelisle removed the review label Dec 18, 2017
@kjdelisle
Copy link
Contributor Author

Note: All I did was rebase manually and merge (the tests all passed beforehand).

@kjdelisle kjdelisle deleted the rest/application branch December 18, 2017 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants