Skip to content

Conversation

@hayemaxi
Copy link
Contributor

@hayemaxi hayemaxi commented Oct 21, 2024

  • NotificationsController
    • Fetches notifications and determines which ones to display.
  • NotificationsNode
    • Side panel that displays a given list (from controller) of notifications.

The controller will fetch notifications from an endpoint (or local files during development). These notifications will be stored in memory and global state. They will then be evaluated for dismissal/criteria and if they pass they will be sent to the NotificationsNode for display.

Extensions will call NotificationController.pollForStartUp() and NotificationController.pollForEmergencies() to get notifications.

image

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions.

@hayemaxi hayemaxi force-pushed the notifications branch 2 times, most recently from bc8edd2 to 736901c Compare October 21, 2024 22:18
- NotificationsController
  - Fetches notifications and determines which ones to display.
- NotificationsNode
  - Side panel that displays a given list (from controller) of notifications.

The controller will fetch notifications from an endpoint (or local files during development).
These notifications will be stored in memory and global state. They will then be evaluated
for dismissal/criteria and if they pass they will be send to the NotificationsNode for display.
@hayemaxi hayemaxi marked this pull request as ready for review October 22, 2024 15:43
@hayemaxi hayemaxi requested review from a team as code owners October 22, 2024 15:43
@@ -0,0 +1,9 @@
/*!
Copy link
Contributor

Choose a reason for hiding this comment

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

somewhat off-topic, but would it avoid boilerplate if the main/top-level core index.ts file instead just exports all notifications stuff under a notifications item? that has at least these advantages:

  • avoids needing to update package.json
  • avoids needing to create this file
  • centralizes the index
  • avoids consumers having to hunt for index files (they import one "mega index")

Copy link
Contributor Author

@hayemaxi hayemaxi Oct 23, 2024

Choose a reason for hiding this comment

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

In addition to a top-level index.js, you would still need a lower level one to collect the individual files though right?
e.g.

src/notifications/index.ts

export * from 'controller.ts'
export * from 'types.ts'

src/index.ts

export * as notifications from 'notifications/index'

Maybe you could do this, but this looks like it would be very messy for many exports:

src/index.ts

import * as controller from './src/notifications/controller';
import * as types from './src/notifications/types';

export const notifications = {
  ...controller,
  ...types,
}

Also, not sure how autocomplete will behave. Needs investigation.

Copy link
Contributor

Choose a reason for hiding this comment

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

you would still need a lower level one to collect the individual files though right?
e.g.

no, could do that manually in the top level index.ts like this:

export * as messages from './utilities/messages'
export * as errors from './errors'

of course, this means the top level has to "choose" some names, but in practice that doesn't seem to be a problem, and it may actually lead to a better interface for the consumers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So then you have pretty granular exports anyways, right? e.g.

export * as notificationsTypes from './src/notifications/types'
export * as notificationsRules from './src/notifications/rules'
export * as notificationsController from './src/notifications/controller'

which isn't as intuitive as a single notifications export from all of these places.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, though it depends on what consumers actually need. Generally we shouldn't need to expose most modules in the "public" interface.

Tracked in https://taskei.amazon.dev/tasks/IDE-15174

@hayemaxi hayemaxi merged commit 41f64ae into aws:master Oct 25, 2024
21 of 24 checks passed
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.

4 participants