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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rest-crud): add CrudRestApiBuilder #4589

Merged
merged 1 commit into from Feb 20, 2020
Merged

feat(rest-crud): add CrudRestApiBuilder #4589

merged 1 commit into from Feb 20, 2020

Conversation

@nabdelgadir
Copy link
Contributor

nabdelgadir commented Feb 7, 2020

Closes #3737

CrudRestApiBuilder is a model API builder that builds the default repository and controller class for a given Entity class.

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 馃憟

@nabdelgadir nabdelgadir self-assigned this Feb 7, 2020
@nabdelgadir nabdelgadir added the feature label Feb 7, 2020
@nabdelgadir nabdelgadir marked this pull request as ready for review Feb 7, 2020
Copy link
Contributor

agnes512 left a comment

Great job! I have a few questions and suggestions.

Copy link
Contributor

jannyHou left a comment

@nabdelgadir Good stuff 馃憤 I post a few comments, mostly LGTM.

inject(`datasources.${modelConfig.dataSource}`)(
repositoryClass,
undefined,
0,

This comment has been minimized.

Copy link
@jannyHou

jannyHou Feb 11, 2020

Contributor

@nabdelgadir might be a silly question: I am looking at the inject function, how do we know the index is 0 and why the member name is undefined?

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Feb 13, 2020

Author Contributor

From my understanding, the only reason we're using a number (that happens to be 0 in this case) is so that it goes into this if statement, but I don't think the number actually matters. And we're only passing in member since it's required and so we're able to pass in the number as the third argument, so leaving it as undefined is the easiest way to go about it (but I don't actually know the role of member in the injection, maybe @bajtos could comment on this and why we're passing in 0?).

This comment has been minimized.

Copy link
@emonddr

emonddr Feb 13, 2020

Contributor

@bajtos ^ (I have the same question)

This comment has been minimized.

Copy link
@bajtos

bajtos Feb 13, 2020

Member

When you set the member to undefined, it means the constructor function. (The constructor does not have any name.

The index 0 is identifying the first argument of the target function (the class constructor in this case).

In this particular invocation of inject, we are decorating the first argument of the repository constructor to inject the datasource instance. It's an equivalent of the following TypeScript code:

class MyRepository /*extends ...*/ {
  constructor(
    @inject(`datasources.${modelConfig.dataSource}`)
    public dataSource: juggler.DataSource
  ) {}
}

Because decorators are converted to JavaScript calls of TypeScript API at compile time, we cannot use decorator syntax when building the repository class dynamically at runtime, we have to invoke the decorator function manually.

This comment has been minimized.

Copy link
@emonddr

emonddr Feb 13, 2020

Contributor

Can we create a constant and use that instead of 0? Perhaps also one for the undefined value? So both constants can provide some clarity to the method call?

inject(`datasources.${modelConfig.dataSource}`)(
    repositoryClass,
    UNDEFINED_CLASS_CONSTRUCTOR_FUNCTION,
    INDEX_OF_INJECTED_DATASOURCE_PARAMETER_INSIDE_REPOSITORY_CONSTRUCTOR,

I know the names are silly, but you get the point.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Feb 20, 2020

Contributor

Because decorators are converted to JavaScript calls of TypeScript API at compile time, we cannot use decorator syntax when building the repository class dynamically at runtime, we have to invoke the decorator function manually.

Thanks everyone for the reply. This is something I didn't know before 馃憤

Copy link
Contributor

emonddr left a comment

great start, nora


if (!(modelClass.prototype instanceof Entity)) {
throw new Error(
`CrudRestController requires an Entity, Models are not supported. (Model name: ${modelName})`,

This comment has been minimized.

Copy link
@emonddr

emonddr Feb 12, 2020

Contributor

CrudRestController requires an Entity, Models are not supported. (Model name: ${modelName}),

What are we trying to say here? That the model needs to extend Entity?

Perhaps

CrudRestController requires a model that extends 'Entity'. (Model name: ${modelName} does not extend 'Entity')

This comment has been minimized.

Copy link
@emonddr

emonddr Feb 12, 2020

Contributor

Why are we calling the file 'plugin'?

packages/rest-crud/src/crud-rest-builder.plugin.ts

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Feb 13, 2020

Author Contributor

Why are we calling the file 'plugin'?

packages/rest-crud/src/crud-rest-builder.plugin.ts

Following the terminology in the spike

This comment has been minimized.

Copy link
@emonddr

emonddr Feb 13, 2020

Contributor

@nabdelgadir ,

Hmm. Let's not use the word plugin. We don't mention it in the overall LB4 docs.

As an example, AuthenticationStrategyProvider is a class that defines an extension point where developers can create extensions (custom authentication strategies) and register them against the extension point... but we don't call any of the files .plugin.ts .

Same for the extensions for greeter extension point : https://github.com/strongloop/loopback-next/blob/master/examples/greeter-extension/src/greeters/greeter-en.ts#L6 , we don't use .plugin.ts .

https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/providers/auth-strategy.provider.ts#L28

This comment has been minimized.

Copy link
@bajtos

bajtos Feb 13, 2020

Member

Let's not use the word plugin. We don't mention it in the overall LB4 docs.

Considering that we are using ExtensionPoint/Extension design pattern, maybe we can use "extension" instead of "plugin"?

This comment has been minimized.

Copy link
@bajtos

bajtos Feb 14, 2020

Member

On the other hand, since this file contains CrudRestApiBuilder class which is bound asModelApiBuilder, maybe crud-rest.model-api-builder.ts would be a good name, using model-api-builder as the type suffix (similar to controller and repository we use elsewhere). If model-api-builder is too long, the perhaps api-builder is good enough too? (I.e. crud-rest.api-builder.ts).

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Feb 17, 2020

Author Contributor

I like crud-rest.api-builder.ts because it's shorter (and the builder is CrudRestApiBuilder instead of CrudRestModelApiBuilder, so the Model part is implied).

packages/rest-crud/src/crud-rest-builder.plugin.ts Outdated Show resolved Hide resolved
@nabdelgadir nabdelgadir force-pushed the crud-rest branch from bcc3000 to 0b75cca Feb 13, 2020
};
```

Now your `Product` model will have a default repository and default controller

This comment has been minimized.

Copy link
@emonddr

emonddr Feb 13, 2020

Contributor

So there are 2 ways that a developer can create the default controller and default repository for Product?

  1. Performing Basic steps and not performing Advanced steps
  2. Performing Advanced steps and not performing Basic steps

So they are mutually exclusive then? If so let's specify that Basic and Advanced are mutually exclusive. Another question: why would someone want to do Advanced steps if it is shorter to perform Basic steps?

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Feb 17, 2020

Author Contributor

So they are mutually exclusive then? If so let's specify that Basic and Advanced are mutually exclusive.

Yeah, I'll specify that, thanks!

Another question: why would someone want to do Advanced steps if it is shorter to perform Basic steps?

The advanced one helps if you only want to create the default controller or default repository but not both.

Copy link
Member

bajtos left a comment

Great work, @nabdelgadir 鉂わ笍

Let's make the implementation easier to understand for all maintainers, including our future selves - see my comments bellow.

@nabdelgadir nabdelgadir force-pushed the crud-rest branch from 0b75cca to 5aef80d Feb 17, 2020
@nabdelgadir nabdelgadir requested review from emonddr and bajtos Feb 18, 2020
`@loopback/rest-crud` exposes two helper methods (`defineCrudRestController` and
`defineCrudRepositoryClass`) for creating controllers and respositories using
code.
`@loopback/rest-crud` can be used along with `@loopback/model-api-builder` to

This comment has been minimized.

Copy link
@bajtos

bajtos Feb 18, 2020

Member

I think this is not entirely correct. @loopback/model-api-builder provides only Types and helpers for packages contributing Model API builders (quoting from README).

Did you perhaps mean the built-in ModelApiBooter?

Suggested change
`@loopback/rest-crud` can be used along with `@loopback/model-api-builder` to
`@loopback/rest-crud` can be used along with the built-in `ModelApiBooter` to

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Feb 19, 2020

Author Contributor

In this case, CrudRestApiBuilder implements ModelApiBuilder (not the booter). Quoting from the README: it also "provides a contract for extensions that contribute builders for repositories and controllers".

@bajtos
bajtos approved these changes Feb 18, 2020
Copy link
Member

bajtos left a comment

The code looks great now!

It would be great to add a test to verify the scenario when the booter detects an existing repository class and does not define a new one, see #4589 (comment).

I have few minor comments, see below.

Please allow some time for other reviewers to have a chance to comment on the new version before landing.

packages/rest-crud/src/crud-rest.api-builder.ts Outdated Show resolved Hide resolved
packages/rest-crud/src/crud-rest.api-builder.ts Outdated Show resolved Hide resolved
packages/rest-crud/src/crud-rest.api-builder.ts Outdated Show resolved Hide resolved
packages/rest-crud/src/crud-rest.api-builder.ts Outdated Show resolved Hide resolved
@nabdelgadir nabdelgadir force-pushed the crud-rest branch 2 times, most recently from 7c822d3 to bd0e910 Feb 19, 2020
`CrudRestApiBuilder` is a model API builder that builds the default repository and controller class for a given Entity class.

Co-authored-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
@nabdelgadir nabdelgadir force-pushed the crud-rest branch from bd0e910 to 5286f45 Feb 20, 2020
@nabdelgadir nabdelgadir merged commit bc5d56f into master Feb 20, 2020
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.02%) to 92.541%
Details
@nabdelgadir nabdelgadir deleted the crud-rest branch Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can鈥檛 perform that action at this time.