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

feat: introduce module coordinator #47

Merged
merged 1 commit into from
Feb 4, 2022
Merged

feat: introduce module coordinator #47

merged 1 commit into from
Feb 4, 2022

Conversation

diehbria
Copy link
Contributor

@diehbria diehbria commented Feb 4, 2022

Overview

  • add first pass in creating appKitSession
  • change subscribeToDataStream to subscribeToTimeSeriesData
  • restructure the core folder structure, partially.
  • add typing to various tests and mocks
  • add some 'jest integration' tests to the asset source
  • fix aggregated data parsing - was incorrectly always putting data in the "raw" data field, data
  • remove unused styles in core
  • mark deprecated mocks as deprecated within core
  • move mock data source into testing
  • refactor the data source into source specific folders
  • add some tests in asset session
  • change asset module mocks.spec.ts to mocks.ts as it wasn't actually a test file. later we will move this to the testing sub folder

Parts contributed by @fpauer

  • refactor sitewise resource explorer to utilize app kit session
  • refactor app kit session to include the site wise asset tree
  • refactor resource explorer unit tests

Legal

This project is available under the Apache 2.0 License.

@diehbria diehbria force-pushed the coord-refact branch 2 times, most recently from e23633f to 65cee36 Compare February 4, 2022 00:24
@diehbria diehbria marked this pull request as draft February 4, 2022 16:32
@diehbria diehbria force-pushed the coord-refact branch 2 times, most recently from b0def83 to 5b019de Compare February 4, 2022 17:07
@NorbertNader NorbertNader requested review from NorbertNader and removed request for gareth-amazon February 4, 2022 17:35
@diehbria diehbria marked this pull request as ready for review February 4, 2022 17:39
update: (subscriptionUpdate: SubscriptionUpdate<SiteWiseDataStreamQuery>) => void;
};
iotsitewise: {
subscribeToAssetTree: (query: SiteWiseAssetTreeQuery, callback: SiteWiseAssetTreeCallback) => AssetTreeSubscription;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is going to be refactored again. But the intention is not to require a Query to get a Session.

Maybe I should get rid of the "Query" language here and move this over to subscribe/promise methods like the core module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, this is definitely incremental, I'm going to hand off that portion to @boweihan to finish off :-)

import { DescribeAssetModelResponse } from '@aws-sdk/client-iotsitewise';
import { completeDataStreams } from '../../completeDataStreams';

export const subscribeToTimeSeriesData =
Copy link
Contributor

Choose a reason for hiding this comment

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

To my mind, this is the definition of a Query implementation. We will just make this produce a Provider<> in the refactor.

gareth-amazon
gareth-amazon previously approved these changes Feb 4, 2022
fpauer
fpauer previously approved these changes Feb 4, 2022
NorbertNader
NorbertNader previously approved these changes Feb 4, 2022
boweihan
boweihan previously approved these changes Feb 4, 2022
Copy link
Contributor

@boweihan boweihan left a comment

Choose a reason for hiding this comment

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

LGTM

export * from './iotsitewise/time-series-data/types.d';

export type IoTAppKitSession = {
subscribeToTimeSeriesData: (
Copy link
Contributor

Choose a reason for hiding this comment

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

As Gareth has pointed out, we'll be changing this class to only handle registering data modules and metrics. This is good incremental progress towards that!


if (input.registerDataSources !== false) {
/** Automatically registered data sources */
dataModule.registerDataSource(createDataSource(siteWiseSdk));
Copy link
Contributor

Choose a reason for hiding this comment

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

Next step, support initializing with multiple data modules that each have their own credential schemes :)


const { update, unsubscribe } = dataModule.subscribeToDataStreams({ queries, request }, (updatedDataStreams) => {
dataStreams = updatedDataStreams;
emit();
Copy link
Contributor

Choose a reason for hiding this comment

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

data and asset requests are fired and emitted independently here. Would it be a better chart experience to guarantee simultaneous initial data delivery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the end goal is to make it configurable on how this behavior is. consider this PR as part one. There's a couple things missing, that is one of them.

* fix aggregated data parsing
* remove unused styles in core
* mark deprecated mocks as deprecated within core
* move mock data source into testing
* refactor the data source into source specific folders
* add some tests in asset session
@diehbria diehbria merged commit 3fb5076 into main Feb 4, 2022
@diehbria diehbria deleted the coord-refact branch February 4, 2022 18:18
sheilaXu pushed a commit that referenced this pull request Sep 23, 2022
…G files (#47)

Co-authored-by: Mitchell Lee <mitchlee@amazon.com>
TheEvilDev pushed a commit that referenced this pull request Nov 2, 2022
* fix aggregated data parsing
* remove unused styles in core
* mark deprecated mocks as deprecated within core
* move mock data source into testing
* refactor the data source into source specific folders
* add some tests in asset session
This was referenced Nov 2, 2022
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.

None yet

5 participants