-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
docs: update dependency injection examples #3024
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emonddr I added one more thing to update, otherwise LGTM good update!
The test failures should be fixed after a rebase.
} | ||
server | ||
.bind(JWTAuthenticationStrategyBindings.TOKEN_EXPIRES_IN) | ||
.to('600'); | ||
``` | ||
|
||
However, when you want to create a binding that will instantiate a class and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emonddr One more thing to update here:
However, when you want to create a binding that will instantiate a configured class(as a provider) and automatically inject required dependencies, then you need to use .toProvider() method:
server
.bind(AuthenticationBindings.AUTH_ACTION)
.toProvider(AuthenticateActionProvider);
const provider = await server.get(AuthenticationBindings.AUTH_ACTION);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jannyHou , the existing document used the AuthenticateActionProvider in a .toClass() and in the .toProvider() example.
I've changed the .toClass() example code to use a different class instead; so 1) it is less confusing as to why different binding methods were used on the same class, and 2) so I don't have to change the explanatory paragraph:
In the longer term, I wonder if we should use simpler examples at https://github.com/strongloop/loopback-next/tree/master/examples/context for the DI docs. |
4a019c5
to
1903e4b
Compare
Hi Raymond, with the examples I have in place at the moment of my 2nd commit, are you satisfied with the 3 different examples (for the purposes of the authentication documentation changes), and want to create a separate task for modifying these dependency injection examples to be done at a later time? Or are you saying that my PR should use non-authentication examples right now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM please rebase the branch before merging.
1903e4b
to
36d480c
Compare
I'm fine to use authentication examples for now. It would be ideal to use simpler standalone examples in the future PRs. |
Me too. Ok thanks. Can you approve it then? And then I will rebase, etc. Thanks :) |
ddace71
to
3a4b1f7
Compare
6e0ee45
to
6620e39
Compare
Update dependency injection examples that mention authentication
6620e39
to
cb95e91
Compare
Update dependency injection examples that mention authentication
Related to : #2940
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈