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

Revert driver initialization logic and adapt Notifications cache #16677

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

sebastienros
Copy link
Member

Fixes #16673

Checked that notifications are cached.
Checked that the submitted sample code in the bug worked.

@sebastienros
Copy link
Member Author

@Habbni if you want to check this branch.

.Take(_notificationOptions.TotalUnreadNotifications + 1)
.ListAsync()).ToList();
var result = Initialize<UserNotificationNavbarViewModel>("UserNotificationNavbar")
.Processing<UserNotificationNavbarViewModel>(async model =>
Copy link
Member

Choose a reason for hiding this comment

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

@sebastienros if we can't get the Cache() to work on the on the ShapeResult, shouldn't we remove it then?

Also, since we are changing Initilize().Processing() should probably already take the same TModel instead of having to pass that again. It just seems weird to me to have to pass the type multiple times in the same chain.

Copy link
Member Author

Choose a reason for hiding this comment

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

if we can't get the Cache() to work on the on the ShapeResult, shouldn't we remove it then

This works

should probably already take the same TModel

I didn't want to introduce a breaking change, or more involved one, as it would require a new ShapeResult and other related changes. This change works.

Copy link
Member

Choose a reason for hiding this comment

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

So the caching work, but to avoid initializing on every request, we have to delegate the initialize logic to the Processing method. Correct? If so, I am not sure the API is intuitive enough. My first instinct would be to initialize as I would normally do in the call back.

It would been much better if we did not have to explain the use of Processing method in this case as it complicate the code.

@Habbni
Copy link
Contributor

Habbni commented Sep 6, 2024

@Habbni if you want to check this branch.

works perfectly. To be extra safe, i just applied it to a current project where we quite heavily customize the admin area with those methods and everything looks like expected now..

@MikeAlhayek MikeAlhayek merged commit ed706e9 into main Sep 6, 2024
22 checks passed
@MikeAlhayek MikeAlhayek deleted the sebros/processasync branch September 6, 2024 16:23
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.

OnDisplaying in custom ShapeTableProvider does not populate data anymore in some cases
3 participants