Skip to content

Comments

Injected properties are expected to be available#24685

Merged
guardrex merged 3 commits intomainfrom
guardrex-patch-2
Jan 20, 2022
Merged

Injected properties are expected to be available#24685
guardrex merged 3 commits intomainfrom
guardrex-patch-2

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
```

> [!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!

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