Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Dependency injection writeup #333
Dependency injection writeup #333
Changes from 18 commits
d1a5a6f
90a42c4
2d1dd54
dee52de
48653ee
44ec179
8ce8e6f
6a054c5
12cb5a2
4124dfa
e279e7f
5fd9129
47b958a
4039dca
7e9c0ad
b33aa4f
8390c86
7004b34
b3725a3
8cdf1c2
ecb5d44
5cc7373
03095e5
8f824e5
7ea529f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
🎨 suggestion: mention the team-specific libraries e.g.
libs/auth
,libs/vault
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.
💭 thought: we should keep an eye on microsoft/TypeScript#4881 which might be able to fix this
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.
Nice spot!
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.
Keeping this open for future reference! :)
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.
suggestion(non-blocking): preferably in standalone components or using
provideIn: 'root'
and not in the actual feature moduleThere 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.
Why not? If the component is standalone, then by definition it doesn't have a module, so advice about a feature module doesn't apply. And if you have a service that is only used in a feature module, I assume you'd want to register it in the more specific location rather than in root. Does this go to your earlier concern about accidentally instantiating duplicate services?
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.
Good point, I read
feature module
as just a folder in an organize-by-feature structure. I think feature modules could (and should) in most cases be replaced by standalone component, but that discussion might be outside the scope of this PR comment :DYes, and other things. It depends a little bit on how we define a "feature". If
feature => 1 exported top-level component
then we are basically talking about standalone components.In the case where 1 feature module exports multiple components then the module becomes a bit rigid. It's a big blob containing all of the dependencies that any of it's child components might need. It makes it less obvious which component needs which service/pipe/etc. and also affects tree-shaking negatively. The lifetime of the service also becomes much easier to reason about, since it's tied to an actual DOM component, instead of a module that is somewhat magically instantiated by Angular in the background. Service tied to components also have access to the activated route in a way that service in modules never will (though this use-case might be a bit of an edge case).
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.
I added a mention of standalone components when discussing the web vault client, otherwise I suggest this could be done in a subsequent revision of this page - which would definitely be welcome. It might require some discussion/consensus on stricter rules around feature modules though. I agree in many cases they can be replaced with standalone, but it's not what we do today.