Skip to content

Conversation

@shazha
Copy link
Contributor

@shazha shazha commented Sep 12, 2018

Document the need for extend the application with ServiceMixin and fix two typos in service booter
doc

re #1701

Checklist

  • 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

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Just one minor change needed and this should be good to land.

```ts
import {ServiceMixin} from '@loopback/service-proxy';

export class StorageApplication extends BootMixin(
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be TodoListApplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 😄

| `dirs` | `string \| string[]` | `['services']` | Paths relative to projectRoot to look in for Service artifacts |
| `extensions` | `string \| string[]` | `['.service.js']` | File extensions to match for Service artifacts |
| `nested` | `boolean` | `true` | Look in nested directories in `dirs` for Service artifacts |
| `glob` | `string` | | A `glob` pattern string. This takes precedence over above 3 options (which are used to make a glob pattern). |
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for cleaning up the mess I left after copy-and-paste 👍

}
```

### Extend application with ServiceMixin
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we need this step?

Since #1649, applications scaffolded via lb4 include the ServiceMixin automatically, unless the user chooses to opt-out.

Should we perhaps rephrase this section and explain that it applies only to applications that were initially scaffolded without services?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, great catch!! In light of this I think I would like to not see this section added to the docs since we are trying to ready the docs for GA.

@shazha Can you update this to actually just remove the Installed @loopback/service-proxy section. The latest version of @loopback/cli scaffolds an app correctly and should not require installing / modifying anything else on the part of a user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ServiceMixin is indeed enabled by default. My own application was generated by a earlier version lb4, I should've tested it against the latest cli.
I've removed the newly added section, as well as the Install @loopback/service-proxy section mentioned by @virkt25 .

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.

👍

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.

👏

ServiceMixin is installed by CLI scaffolding. Fix two typos in service booter
doc

re #1701
@virkt25 virkt25 merged commit f5cd069 into loopbackio:master Sep 13, 2018
@virkt25
Copy link
Contributor

virkt25 commented Sep 13, 2018

You PR has landed. Thankyou for the contribution @shazha!! 🎉 💯

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