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

v.3.2.0 #46

Merged
merged 10 commits into from
Mar 27, 2019
Merged

v.3.2.0 #46

merged 10 commits into from
Mar 27, 2019

Conversation

michaelyali
Copy link
Member

@michaelyali michaelyali commented Mar 25, 2019

Features:

  • controller - added UsePathInterceptors decorator that allows to wire up RestfulQueryInterceptor and RestfulParamsInterceptor on custom CrudController routes (those interceptors are already wired up on @Override() routes) (How to reuse queries in 3.x? #45)
  • repository service - added simple findOne and find methods (link to the repo methods)

@michaelyali michaelyali requested a review from Diluka March 25, 2019 13:59
Copy link
Contributor

@Diluka Diluka left a comment

Choose a reason for hiding this comment

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

LGTM

@Diluka
Copy link
Contributor

Diluka commented Mar 26, 2019

@zMotivat0r wait a minute. Does it support 5.x?

@michaelyali
Copy link
Member Author

@zMotivat0r wait a minute. Does it support 5.x?

5.x of what? NestJs?

@michaelyali michaelyali requested a review from Diluka March 26, 2019 08:34
@Diluka
Copy link
Contributor

Diluka commented Mar 26, 2019

Yes, I mean NestJS. And I find it only supporting 6 and above. I suggest to define the peerDependencies

Copy link
Contributor

@Diluka Diluka left a comment

Choose a reason for hiding this comment

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

Seems OK

@Diluka
Copy link
Contributor

Diluka commented Mar 26, 2019

protected options: RestfulOptions = {};

Is there necessary to have 2 options separated.

It's uncontrollable in some scenarios. For example:
someone override controller#getManyBase, modify options and call service#getMany.
but options maybe changed in service#getMany and behavior unexpected.

public async getMany(
query: RequestParamsParsed = {},
options: RestfulOptions = {},
): Promise<GetManyDefaultResponse<T> | T[]> {
const builder = await this.buildQuery(query, options);
const mergedOptions = Object.assign({}, this.options, options);

@michaelyali
Copy link
Member Author

@Diluka what's your suggestion?

@Diluka
Copy link
Contributor

Diluka commented Mar 27, 2019

A few questions about this

  1. Why there has options property on service?
  2. Can options merged in interceptor?

My suggestion is
service#getMany and other methods just do CRUD work. Do not modify options in those methods.

@michaelyali
Copy link
Member Author

  1. Initially, it was designed to place some common options in a service rather than duplicating in several controllers if you have few controllers for one service (e.g. maxLimit, cache).
  2. Haven't tried that yet. Need to think about it.

Anyways, it will be another breaking change with major version bump.
My suggestion is to release this and to find a way how to have common options in one place (e.g. maxLimit, cache) without merging options in a service.

@Diluka
Copy link
Contributor

Diluka commented Mar 27, 2019

@zMotivat0r OK

@michaelyali
Copy link
Member Author

I just forgot to add changes to README :(

@michaelyali michaelyali merged commit a6a5f3d into master Mar 27, 2019
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.

2 participants