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.

Sign up for GitHub

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

Jump to bottom

refactor(rest): use dynamic value provider for actions #5561

Merged
merged 1 commit into from
Oct 30, 2020
Merged

refactor(rest): use dynamic value provider for actions #5561

merged 1 commit into from
Oct 30, 2020

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented May 26, 2020 •
edited
Loading

Optimize actions using static providers.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

bajtos
bajtos previously requested changes May 26, 2020
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

This looks like a breaking change to me because AFAICT, FindRouteProvider is a public API exported by @loopback/rest. (Strangely enough, other sequence action providers are not exported.)

Please check other touched packages for breaking changes in the public API.

Have you considered preserving current instance-based providers as thin wrappers around the new static providers for backwards compatibility? Maybe it's not worth the effort, let's discuss.

If we are going to make breaking changes, then let's check what other changes to include in the major release please. For example, it would be great to remove support for externally hosted REST API Explorer from @loopback/rest so bring us closer to shutting down https://explorer.loopback.io

@bajtos
Copy link
Member

bajtos commented May 26, 2020

I don't have strong opinions on the actual change from instance providers to static providers, I'll leave that up to code owners to decide (@hacksparrow @emonddr @deepakrkris).

emonddr
emonddr suggested changes May 26, 2020
View reviewed changes
Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

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

This is the first time I am seeing static value method for providers. Just wondering: 1) the strategic reason for making them static, and 2) how this will affect existing applications using components affected by this change.

@raymondfeng
Copy link
Contributor Author

@emonddr See #5370

@raymondfeng raymondfeng requested a review from emonddr May 26, 2020 17:27
@raymondfeng
Copy link
Contributor Author

Have you considered preserving current instance-based providers as thin wrappers around the new static providers for backwards compatibility? Maybe it's not worth the effort, let's discuss.

That's probably not worth the effort. Too many choices are bad :-).

@raymondfeng raymondfeng force-pushed the action-as-static-providers branch 2 times, most recently from 0d5e551 to 5b6a1ce Compare June 2, 2020 01:55
@raymondfeng raymondfeng force-pushed the action-as-static-providers branch from 5b6a1ce to 26bc0b5 Compare July 27, 2020 16:23
@raymondfeng raymondfeng force-pushed the action-as-static-providers branch from 26bc0b5 to 6ecd5bc Compare August 4, 2020 20:14
@raymondfeng raymondfeng requested a review from jannyHou as a code owner August 4, 2020 20:14
@raymondfeng raymondfeng force-pushed the action-as-static-providers branch 2 times, most recently from 1fcaf2b to 68b6631 Compare August 13, 2020 22:00
@bajtos
Copy link
Member

bajtos commented Aug 17, 2020

Have you considered preserving current instance-based providers as thin wrappers around the new static providers for backwards compatibility? Maybe it's not worth the effort, let's discuss.

That's probably not worth the effort. Too many choices are bad :-).

That's true 👍

Please update the commit message to communicate this change as backwards-incompatible.

@bajtos bajtos dismissed their stale review August 17, 2020 15:03

I'll leave the review in the hands of code owners.

@raymondfeng raymondfeng force-pushed the action-as-static-providers branch 2 times, most recently from dab0860 to fbe8fd2 Compare August 20, 2020 22:06
@raymondfeng raymondfeng force-pushed the action-as-static-providers branch from fbe8fd2 to 40b52c6 Compare August 27, 2020 22:19
@raymondfeng
Copy link
Contributor Author

@jannyHou @hacksparrow Please review.

@raymondfeng raymondfeng added breaking-change REST Issues related to @loopback/rest package and REST transport in general labels Aug 27, 2020
@raymondfeng raymondfeng force-pushed the action-as-static-providers branch from 40b52c6 to 5692a84 Compare September 30, 2020 16:08
@dhmlau dhmlau added this to the Oct 2020 milestone Oct 14, 2020
@dhmlau dhmlau assigned hacksparrow and jannyHou and unassigned jannyHou Oct 19, 2020
jannyHou
jannyHou reviewed Oct 30, 2020
View reviewed changes
packages/rest/src/providers/send.provider.ts
export class SendProvider implements Provider<Send> {
value() {
export class SendProvider {
static value() {
Copy link
Contributor

Choose a reason for hiding this comment

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

not necessarily in this PR, can we also turn SendResponseMiddlewareProvider into a static Provider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed as SendResponseMiddlewareProvider is a singleton.

Copy link
Contributor

Choose a reason for hiding this comment

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

@raymondfeng the previous SendProvider is also a singleton...

@raymondfeng raymondfeng force-pushed the action-as-static-providers branch from 5692a84 to ad6173b Compare October 30, 2020 16:59
jannyHou
jannyHou reviewed Oct 30, 2020
View reviewed changes
packages/rest/src/providers/invoke-method.provider.ts
static value(
@inject(RestBindings.Http.CONTEXT) context: Context,
): InvokeMethod {
const invokeMethod: InvokeMethod = (route, args) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

types for route and args are missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are inferred from InvokeMethod type.

jannyHou
jannyHou reviewed Oct 30, 2020
View reviewed changes
packages/rest/src/providers/find-route.provider.ts
@inject(RestBindings.Http.CONTEXT) context: Context,
@inject(RestBindings.HANDLER) handler: HttpHandler,
): FindRoute {
const findRoute: FindRoute = request => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: missing Request type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's inferred from FindRoute.

jannyHou
jannyHou approved these changes Oct 30, 2020
View reviewed changes
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.

According to the benchmark this PR would have a significant improvement for request processing 👍

A few nitpicks and please fix the lint.

@raymondfeng
refactor(rest): use dynamic value provider for actions
eac9a22 
BREAKING CHANGE: If you use one of the built-in action providers as the base
class, this commit will break you as the signature of the base class has
changed. Otherwise the code should be backward compatible for existing
applications.

Signed-off-by: Raymond Feng <[email protected]>
@raymondfeng raymondfeng force-pushed the action-as-static-providers branch from ad6173b to eac9a22 Compare October 30, 2020 18:56
@raymondfeng
Copy link
Contributor Author

According to the benchmark this PR would have a significant improvement for request processing

This one is minor improvement.

@raymondfeng raymondfeng merged commit 3a32290 into master Oct 30, 2020
@raymondfeng raymondfeng deleted the action-as-static-providers branch October 30, 2020 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jannyHou jannyHou jannyHou approved these changes

@bajtos bajtos bajtos left review comments

@deepakrkris deepakrkris Awaiting requested review from deepakrkris

@hacksparrow hacksparrow Awaiting requested review from hacksparrow

@emonddr emonddr Awaiting requested review from emonddr

Assignees

@hacksparrow hacksparrow

Labels
breaking-change REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Milestone
Oct 2020
Development

Successfully merging this pull request may close these issues.
6 participants
@raymondfeng @bajtos @emonddr @jannyHou @hacksparrow @dhmlau