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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Injected properties are expected to be available #24685

Merged
merged 3 commits into from
Jan 20, 2022
Merged

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Jan 20, 2022

Addresses #24615

Per dotnet/aspnetcore#39445 (comment) ...

Pranav, keep/update this? ... or close the PR (e.g., it's too weedy ... too rare)? 👂

Currently, this is only for the 6.0 or later versions due to NRT enforcement for C# 8.0 or later. If we keep/modify this, let me know if you feel that 3.1/5.0 should have this guidance as well. 👂

@guardrex guardrex requested a review from pranavkm January 20, 2022 12:10
@guardrex guardrex self-assigned this Jan 20, 2022
@@ -137,6 +137,21 @@ public class ComponentBase : IComponent
}
```

> [!NOTE]
> Don't mark injected services as nullable. Instead, assign a default literal with the null-forgiving operator (`default!`). For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess. One of the alternatives we were considering was to enable constructor injection by default in 7.0 which would remove some of this ugliness.

Choose a reason for hiding this comment

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

Sorry for the nitpick, but I think it would be best to clarify as to why you shouldn't mark properties as nullable:

Since injected properties are always expected to be available, as a best practice don't mark injected services as nullable. Instead...

The first time I read this, I thought that there might be some more technical reason as to why you shouldn't mark it as nullable.

Copy link
Collaborator Author

@guardrex guardrex Jan 25, 2022

Choose a reason for hiding this comment

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

It's not just a best practice from the remarks made: It's how Blazor is designed to work. Therefore, an imperative remark should be fine.

I do see one thing that I don't care for ... "My"-named things 😩 ... I think I'll patch that out of here. I'll add that bit, @jessegood, when I make that update.

Choose a reason for hiding this comment

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

I see, so as a consumer, we shouldn't think of this as being "optional" at all. Thanks for the clarification!

aspnetcore/blazor/fundamentals/dependency-injection.md Outdated Show resolved Hide resolved
Co-authored-by: Pranav K <prkrishn@hotmail.com>
@guardrex guardrex merged commit ce84790 into main Jan 20, 2022
@guardrex guardrex deleted the guardrex-patch-2 branch January 20, 2022 15:00
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.

3 participants