fix(repository): fix broken code in readme #1121

shimks · 2018-03-12T17:19:34Z

Updated the code to use current practices
Got rid of in-depth explanation of each concepts, but they were good content that might be ok to move out of the readme and into one of the pages in loopback.io. But then again, they may have been covered already. @raymondfeng any thoughts?
connected to Review existing documentation #988

Checklist

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

dhmlau · 2018-03-12T18:59:04Z

packages/repository/README.md

+  id: string;
+  @property() title: string;
+  @property() content: string;
+}
 ```

 **NOTE**: There is no declarative support for data source and model yet in


I thought we have declarative support for data source.
In db.datasource.ts, we can do something like below:

const dsConfigPath = path.resolve('config', 'datasources.json'); const config = require(dsConfigPath); export const db = new DataSourceConstructor(config);

@dhmlau I personally thought that the meaning of having declarative support meant that no config is necessary outside of the declarative file itself (the declarative file being config.json). Please correct me if I am wrong.

I was thinking that we can still define the datasource.json like what we did in LB3. It's not the ultimate goal for declarative support. So i'm not sure what we wanna say about this statement. Maybe @raymondfeng can help :)

dhmlau · 2018-03-12T19:02:05Z

packages/repository/README.md

@@ -21,213 +21,6 @@ $ npm install --save @loopback/repository

 ## Basic use


Can we change it to Basic Use?

@dhmlau In loopback.io, we did sentence-case for titles and I thought we'd be sticking to that in the READMEs as well. Is it different for readmes?

I was looking at the READMEs for @loopback/core and @loopback/rest and they seem to use the title capitalization rules. But i also see that the READMEs are not all consistent. Let me check with @bschrammIBM

won't be a blocker for this PR.

@shimks , according to @bschrammIBM, you're right!

dhmlau · 2018-03-12T19:03:35Z

packages/repository/README.md

 ```

 **NOTE**: There is no declarative support for data source and model yet in
-LoopBack Next. These constructs need to be created programmatically as
+LoopBack 4. These constructs need to be created programmatically as
 illustrated above.

 ### Define a repository


Make it capitalization for titles.
--> Define a Repository

confirmed with @bschrammIBM , your original version was correct (for capitalization) but should be Defining a repository.
Similarly for others.. e.g. Defining a controller.
That's something she can edit later.

dhmlau · 2018-03-12T19:03:52Z

packages/repository/README.md

-  public noteRepo: EntityCrudRepository<Entity, number>;
-}
-```
-
 ### Run the controller and repository together


Same for this one
--> Run the Controller and Repository Together please ignore

dhmlau · 2018-03-12T19:04:04Z

packages/repository/README.md

  }
 }
 ```
+
 ## Related resources


~~Related Resources~~ please ignore

raymondfeng · 2018-03-14T17:08:50Z

@shimks I'm fine to move the concepts into loopback.io. Please add some links to the pages in this README file and make sure all concepts are covered.

shimks · 2018-03-14T18:56:48Z

Since moving the concepts isn't part of the issue #988, I'll keep the key concepts in the readme for now, and then create an issue to implement this decision made here. I'll later reference the issue here once it's been created.

raymondfeng · 2018-03-15T15:49:07Z

packages/repository/README.md


-export const ds: juggler.DataSource = new DataSourceConstructor({
+export const db: juggler.DataSource = new DataSourceConstructor({


Not sure why switch to db. Please note ds stands for dataSource.

I switched the constant name to db because when we import the datasource to be registered with app.datasource(), we retrieve it back through dependency injection via the string: datasources.db and NOT datasources.ds, the name of the constant. I thought we should be consistent here as to not confuse the users.

Alternatively, I could change name: 'db' to name: 'ds' so that the name of the variable being named is consistent with the datasource's name. Should I switch back regardless?

+1 for using db, it's the name of the default we use in our project templates (LB 3.x and 4.x) too.

Not a big deal though, I can live with ds too.

raymondfeng · 2018-03-15T15:49:58Z

packages/repository/README.md

@@ -139,12 +229,13 @@ There are two subtly different types of models for domain objects:
  }
  ```

-### DataSource
+### Datasource


We use DataSource in LB3.

bajtos · 2018-03-19T13:01:22Z

packages/repository/README.md

-It contains the constructs for modeling and accessing data. Repositories can be
-used alone or as part of a `Controller` implementation.
+It contains the constructs for modeling and accessing data. We recommend using
+Repositories with implmentation of `Controllers`.


Typo: implmentation

bajtos · 2018-03-19T13:02:33Z

packages/repository/README.md


-export const ds: juggler.DataSource = new DataSourceConstructor({
+export const db: juggler.DataSource = new DataSourceConstructor({


+1 for using db, it's the name of the default we use in our project templates (LB 3.x and 4.x) too.

Not a big deal though, I can live with ds too.

bajtos · 2018-03-19T13:04:06Z

packages/repository/README.md

+
+  // Create a new note
+  @post('/note')
+  create(@requestBody() data: Note) {


I think data should be Partial<Note>? E.g. when the database is creating the id value for us, then id needs to be omitted from the data payload.

Feel free to leave this change out of scope of this pull request though.

I just tried out changing the type to Partial<Note> to see what OpenAPI spec would get generated, and it just interpreted the parameter type to be an object, so it seems like we can't do this here unless we specifically circumvent around this issue.

This isn't really a problem right now since type validation isn't being done, but it IS technically wrong for the route to accept an object that is only partially a Note. What do you think?

@bajtos ^^^

@shimks I'm interested to hear why the route can't accept Partial<Note>. I thought partial just meant that the type is preserved, but some(or all?) properties are optional.

Yes, Partial<Note> means Note model with all properties made optional. We are already using it in our ORM layer, see e.g. here:

https://github.com/strongloop/loopback-next/blob/1d9979fe4a7d0ef828cd9972694b49be283d7ca8/packages/repository/src/legacy-juggler-bridge.ts#L110-L113

I just tried out changing the type to Partial<Note> to see what OpenAPI spec would get generated, and it just interpreted the parameter type to be an object

That looks like a rather unfortunate limitation of what can be inferred via TypeScript type system 😞

I personally think it's important to have correct type annotation (Partial<Note>), even if it means that we have to explicitly provide the request body schema for OpenAPI Spec.

bajtos · 2018-03-19T13:04:55Z

packages/repository/README.md

+
+  // Find notes by title
+  @get('/note/{title}')
+  findByTitle(@param.query.string('title') title: string) {


Since your path contains {title} in it, you should not use param.query. I think param.path is what you need.

virkt25

Minor nit-picks

virkt25 · 2018-03-22T16:52:39Z

packages/repository/README.md

-It contains the constructs for modeling and accessing data. Repositories can be
-used alone or as part of a `Controller` implementation.
+It contains the constructs for modeling and accessing data. We recommend using
+Repositories with implementation of `Controllers`.


implementation of Controllers but then can be used alone.

I'm a bit confused. Are you saying that I should reword the sentence so that it doesn't imply controllers have to be used in conjunction with repositories?

Yep. Adding a but can be used alone so it's clear our recommendation isn't the only way forward.

virkt25 · 2018-03-22T16:59:31Z

packages/repository/README.md

 - https://martinfowler.com/eaaCatalog/repository.html
 - https://msdn.microsoft.com/en-us/library/ff649690.aspx
 - http://docs.spring.io/spring-data/data-commons/docs/2.0.0.M3/reference/html/#repositories

 ## Contributions

- [Guidelines](https://github.com/strongloop/loopback-next/wiki/Contributing##guidelines)
+- [Guidelines](https://github.com/strongloop/loopback-next/blob/master/docs/site/DEVELOPING.md)


Change this to point to https://github.com/strongloop/loopback-next/blob/master/docs/CONTRIBUTING.md

b-admike · 2018-03-22T19:24:48Z

packages/repository/README.md

-It contains the constructs for modeling and accessing data. Repositories can be
-used alone or as part of a `Controller` implementation.
+It contains the constructs for modeling and accessing data. We recommend using
+Repositories with implementation of `Controllers` but it can be used


It'd be great if we have a sentence on what repositories are before we say We recommend using Repositories...

b-admike · 2018-03-22T19:37:02Z

packages/repository/README.md


-  const repo = new DefaultCrudRepository(Note, ds);
+We'll use `BootMixin` on top of `RepositoryMixin` so that Repository bindings
+can be taken care of at boot.


nitpick: taken care of by @loopback/boot at boot time before the application starts or something along those lines. Feel free to ignore.

b-admike · 2018-03-22T19:47:44Z

packages/repository/README.md

+
+  // Create a new note
+  @post('/note')
+  create(@requestBody() data: Note) {


@shimks I'm interested to hear why the route can't accept Partial<Note>. I thought partial just meant that the type is preserved, but some(or all?) properties are optional.

shimks · 2018-03-22T19:53:48Z

@b-admike It has to do with how TypeScrpt preserves metadata of Constructors that are Partial. When I took a look at the spec being generated from @requestBody() note: Partial<Note>, it had a type property of 'object', meaning its proper schema isn't being referenced.

@bajtos Should I be making an issue to address this problem?

b-admike · 2018-03-22T20:11:43Z

@shimks Thanks for clarifying and +1 for opening an issue to track it.

bajtos · 2018-03-23T12:19:27Z

@shimks

Should I be making an issue to address this problem?

Looks like this issue is trickier than I originally thought. Let's keep you current version create(@requestBody() data: Note) for now and please open a new issue to investigate this in more detail.

For example, I would like to write create(@requestBody(Note) data: Partial<Note>), but somehow tell OpenAPI Spec generator which of Note properties are required, which are optional and which must be excluded from the create request (typically the id property).

bajtos

I think the current proposal is definitely an improvement compared to the current status and should be landed even if it's not perfect.

As far as I am concerned, this patch LGTM.

shimks requested review from kjdelisle and raymondfeng as code owners March 12, 2018 17:19

shimks added the needs-priority label Mar 12, 2018

shimks force-pushed the fix/update-repository-readme branch from 9f3e91c to cb7a989 Compare March 12, 2018 18:55

dhmlau reviewed Mar 12, 2018

View reviewed changes

shimks force-pushed the fix/update-repository-readme branch from 43ee221 to d065b0a Compare March 13, 2018 18:57

shimks force-pushed the fix/update-repository-readme branch from 52041e1 to b45f64b Compare March 14, 2018 19:53

shimks mentioned this pull request Mar 14, 2018

move Concepts section out of @loopback/repository's README and into the docs folder #1137

Closed

6 tasks

shimks force-pushed the fix/update-repository-readme branch from b45f64b to 680c3d1 Compare March 14, 2018 20:46

raymondfeng reviewed Mar 15, 2018

View reviewed changes

shimks force-pushed the fix/update-repository-readme branch from 680c3d1 to 35982af Compare March 15, 2018 16:53

b-admike approved these changes Mar 16, 2018

View reviewed changes

bajtos reviewed Mar 19, 2018

View reviewed changes

shimks force-pushed the fix/update-repository-readme branch 2 times, most recently from cbfbf4d to a1c704f Compare March 21, 2018 14:53

virkt25 reviewed Mar 22, 2018

View reviewed changes

shimks force-pushed the fix/update-repository-readme branch from a1c704f to f290964 Compare March 22, 2018 17:30

b-admike reviewed Mar 22, 2018

View reviewed changes

virkt25 approved these changes Mar 22, 2018

View reviewed changes

b-admike approved these changes Mar 22, 2018

View reviewed changes

bajtos approved these changes Mar 23, 2018

View reviewed changes

shimks force-pushed the fix/update-repository-readme branch 3 times, most recently from cf667a1 to 8d250fe Compare March 23, 2018 15:19

fix(repository): fix broken code in readme

a04fa2d

shimks force-pushed the fix/update-repository-readme branch from 8d250fe to a04fa2d Compare March 23, 2018 15:40

shimks mentioned this pull request Mar 23, 2018

OpenAPI decorator does not properly generate schemas of type Partial #1179

Closed

5 tasks

shimks merged commit e3e97d9 into master Mar 23, 2018

shimks deleted the fix/update-repository-readme branch March 23, 2018 16:12

shimks removed the needs-priority label Mar 23, 2018

		@@ -21,213 +21,6 @@ $ npm install --save @loopback/repository

		## Basic use


		export const ds: juggler.DataSource = new DataSourceConstructor({
		export const db: juggler.DataSource = new DataSourceConstructor({

fix(repository): fix broken code in readme #1121

fix(repository): fix broken code in readme #1121

Conversation

shimks commented Mar 12, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimks Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhmlau Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

dhmlau Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

raymondfeng commented Mar 14, 2018 • edited Loading

shimks commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimks Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimks commented Mar 22, 2018

b-admike commented Mar 22, 2018 • edited Loading

bajtos commented Mar 23, 2018

bajtos left a comment

Choose a reason for hiding this comment

shimks Mar 12, 2018 •

edited

Loading

dhmlau Mar 12, 2018 •

edited

Loading

dhmlau Mar 12, 2018 •

edited

Loading

raymondfeng commented Mar 14, 2018 •

edited

Loading

shimks Mar 19, 2018 •

edited

Loading

b-admike commented Mar 22, 2018 •

edited

Loading