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

fix(rest): Improve rest metadata inheritance #746

Merged
merged 1 commit into from
Nov 16, 2017

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Nov 16, 2017

Make sure metadata inherited from base classes are cloned to avoid
pollution of base information

Description

Related issues

  • connect to <link_to_referenced_issue>

Checklist

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

);

// The parent endpoint has been overridden
expect(childSpec.paths).to.not.have.property('/name');
Copy link
Contributor

Choose a reason for hiding this comment

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

/name ---> /parent-name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

Make sure metadata inherited from base classes are cloned to avoid
pollution of base information
@raymondfeng raymondfeng force-pushed the fix-rest-metadata-inheritance branch from 0b5dce9 to 91e3367 Compare November 16, 2017 21:15
@raymondfeng raymondfeng merged commit 3f124f3 into master Nov 16, 2017
@raymondfeng raymondfeng deleted the fix-rest-metadata-inheritance branch November 16, 2017 21:45
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

LGTM, just had some questions about doc because I think this is a cool feature. They can be addressed in another PR or may not be applicable.

@@ -73,6 +86,20 @@ interface RestEndpoint {
target: any;
}

function getEndpoints(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please have ts docs for this function? (or maybe we don't need it because it's internal?


const {verb, path} = endpoint;
const endpointName = `${fullMethodName} (${verb} ${path})`;
const verb = endpoint.verb!;
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding, what does having a ! at the end of this statement do?

const path = endpoint.path!;

let endpointName = '';
if (debug.enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1


let endpoint: Partial<RestEndpoint> = endpoints[propertyKey];
if (!endpoint) {
const endpoints = getEndpoints(target);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change the tsdocs for this function to say that child classes can override REST operations?


let endpoint: Partial<RestEndpoint> = endpoints[propertyKey];
if (!endpoint) {
const endpoints = getEndpoints(target);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto: should we change ts-docs for this function to say that child classes can override REST parameters?

@@ -26,8 +26,6 @@
"@loopback/context": "^4.0.0-alpha.20",
"@loopback/core": "^4.0.0-alpha.22",
"@loopback/openapi-spec": "^4.0.0-alpha.15",
"@types/http-errors": "^1.5.34",
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be in dependencies because we re-export the http-errors objects for use in @loopback/rest.
Removing this will cause compilation failures downstream!

Copy link
Contributor

@kjdelisle kjdelisle left a comment

Choose a reason for hiding this comment

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

This should not have been merged with the dependency changes to @types/http-errors, as it is now breaking things downstream.

@raymondfeng
Copy link
Contributor Author

Unfortunately, we (myself and reviewers) and the CI build didn't catch the error when it was merged.

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.

4 participants