These should be imported from googlesitekit-notifications, no? We probably need to expose them but otherwise this will get duplicated in the various bundles.

This is interesting. Perhaps I'll create a separate issue as this applies to all refactored notifications and will end up being a larger code change!

Have added #9993 for this. Thanks!

I think we can just return early here rather than wrap everything since all of these notifications would not be registered, correct? If we did have any that should still be registered, we could add them above the return instead.

I would keep this as is only to keep things consistent as this is the pattern we are (and have been) following for all notifications and widgets registration. The additional indentation and wrapper is clearer than the "order" (before/after the feature flag return) especially if the list gets biggers.

I think we may want to consider putting the notification definitions in a separate file(s) similar to how we handle the datastore (you could make the case to do this for widgets too, which we probably should). That way the module index should largely mirror its entrypoint and simply provide exports rather than defining all this inline.

This will be plugin-wide change again. I do sometimes like seeing the "requirements" for the module, its widgets and notifications all in one place. But if we think this is necessary, I can add this requirement to #9993 as an extra bit of refactoring/cleanup?

Yeah I wasn't suggesting we'd do it in this issue, but I do think it would be worth cleaning up the module indexes to be reduced to composition and exports only and organize the implementation into a more logical structure. This should be a separate issue from #9993, potentially multiple issues depending on the effort.

@aaemnnosttv

1. Should I be creating a migration file to update the dismissed prompts key in the DB for users that may have already dismissed the banner? In the new code, the notification ID is used as the dismissal key.
2. Or should we change the notification ID here to be the same as the original dismissal key - but this would be 'hacky' as the ID would have a funny name here. And the GA events will now be tracked to a different ID than the original ID used.
3. Should we keep things as it is and have some of the users who have RRM enabled dismiss the banner again?

I think keeping as-is is the best option here but if the RRM setup prompt is retryable, then I'd be a bit concerned if folks already dismissed it multiple times and now they'd need to dismiss it multiple times again. Is that the case? It looks like yes.

In that case, I think we can simply check for the presence of the old dismissal in the RRM notification's checkRequirements function. That way, everything else runs as usual but we still account for an existing dismissal.

It sounds like a good easy hack which we can remove if we do any migration for dismissed-items or prompts in the future.

I wouldn't say it's a hack 😄

I think keeping as-is is the best option here but if the RRM setup prompt is retryable, then I'd be a bit concerned if folks already dismissed it multiple times and now they'd need to dismiss it multiple times again. Is that the case? It looks like yes.

In that case, I think we can simply check for the presence of the old dismissal in the RRM notification's checkRequirements function. That way, everything else runs as usual but we still account for an existing dismissal.

Do we have an issue for this? We should address this sooner than later and then remove the skipHidingFromQueue option as it shouldn't be needed, correct?

Correct, I've added #10003 to tackle this. Thanks.

Looks like there are styles attached to this selector. Are we sure it's not needed?

I would actually keep the base class if we have a variant specific to the id as well.

Nice 👍 I think we should do this more, at least for the selector/action under test like you've done here. Generally, we should do more destructuring to avoid calling the store select or dispatch repeatedly as it's unnecessary and always makes the code get spread over many more lines.

I wouldn't say it's a hack 😄