Skip to content

Commit 658ca67

Browse files
stephanwleestephanwlee
authored
refactor: use router event to fetch data (again) (#4941)
Revert "Revert "refactor: use router event to fetch data. (#4915)"" This reverts commit 32e533f. Original PR message ================ Context: previously, we were using a composite action[1] to know when the dashboard view updated and thus its data needs to be refreshed. Fix: now that TensorBoard has a router and can determine what views are rendered given a route, we can use a purer redux pattern to determine whether the dashboard data has to be reloaded. Since not all versions of the app uses the router, we will temporarily support the coreLoaded. [1]: composite actioness: view was subscribing to the state changes to determine to fire an action which in turn triggers another action.
1 parent 39d5726 commit 658ca67

26 files changed

+772
-329
lines changed

‎tensorboard/components/experimental/plugin_util/core-host-impl.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {Store} from '@ngrx/store';
2020
import {distinctUntilChanged, filter} from 'rxjs/operators';
2121

2222
import {State} from '../../../webapp/app_state';
23-
import {getAppLastLoadedTimeInMs} from '../../../webapp/selectors';
23+
import {getCoreDataLastLoadedTimeInMs} from '../../../webapp/selectors';
2424
import * as tf_storage_utils from '../../tf_storage/storage_utils';
2525
import {MessageId} from './message_types';
2626
import {Ipc} from './plugin-host-ipc';
@@ -52,7 +52,7 @@ export class PluginCoreApiHostImpl {
5252
});
5353

5454
this.store
55-
.select(getAppLastLoadedTimeInMs)
55+
.select(getCoreDataLastLoadedTimeInMs)
5656
.pipe(
5757
filter((lastLoadedTimeInMs) => lastLoadedTimeInMs !== null),
5858
distinctUntilChanged()

‎tensorboard/components/experimental/plugin_util/plugin_api_host_test.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {of} from 'rxjs';
2020
import {State} from '../../../webapp/app_state';
2121
import {buildRun} from '../../../webapp/runs/store/testing';
2222
import {
23-
getAppLastLoadedTimeInMs,
23+
getCoreDataLastLoadedTimeInMs,
2424
getExperimentIdsFromRoute,
2525
getRuns,
2626
} from '../../../webapp/selectors';
@@ -52,7 +52,7 @@ describe('plugin_api_host test', () => {
5252
store = TestBed.inject<Store<State>>(Store) as MockStore<State>;
5353
store.overrideSelector(getExperimentIdsFromRoute, ['e1']);
5454
store.overrideSelector(getRuns, []);
55-
store.overrideSelector(getAppLastLoadedTimeInMs, null);
55+
store.overrideSelector(getCoreDataLastLoadedTimeInMs, null);
5656
selectSpy = spyOn(store, 'select').and.callThrough();
5757

5858
const ipc = TestBed.inject(Ipc);
@@ -271,19 +271,19 @@ describe('plugin_api_host test', () => {
271271
it('calls callback when last updated time changes', () => {
272272
coreApi.init();
273273

274-
store.overrideSelector(getAppLastLoadedTimeInMs, null);
274+
store.overrideSelector(getCoreDataLastLoadedTimeInMs, null);
275275
store.refreshState();
276276
expect(broadcastSpy).toHaveBeenCalledTimes(0);
277277

278-
store.overrideSelector(getAppLastLoadedTimeInMs, 1);
278+
store.overrideSelector(getCoreDataLastLoadedTimeInMs, 1);
279279
store.refreshState();
280280
expect(broadcastSpy).toHaveBeenCalledTimes(1);
281281

282-
store.overrideSelector(getAppLastLoadedTimeInMs, 1);
282+
store.overrideSelector(getCoreDataLastLoadedTimeInMs, 1);
283283
store.refreshState();
284284
expect(broadcastSpy).toHaveBeenCalledTimes(1);
285285

286-
store.overrideSelector(getAppLastLoadedTimeInMs, 2);
286+
store.overrideSelector(getCoreDataLastLoadedTimeInMs, 2);
287287
store.refreshState();
288288
expect(broadcastSpy).toHaveBeenCalledTimes(2);
289289
});

‎tensorboard/webapp/BUILD

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,6 @@ tf_ng_module(
107107
"//tensorboard/webapp/app_routing:route_registry",
108108
"//tensorboard/webapp/app_routing/views",
109109
"//tensorboard/webapp/core",
110-
"//tensorboard/webapp/core/actions",
111-
"//tensorboard/webapp/core/store",
112110
"//tensorboard/webapp/core/views:hash_storage",
113111
"//tensorboard/webapp/core/views:page_title",
114112
"//tensorboard/webapp/experiments",
@@ -124,29 +122,6 @@ tf_ng_module(
124122
"//tensorboard/webapp/tb_wrapper",
125123
"@npm//@angular/core",
126124
"@npm//@angular/platform-browser",
127-
"@npm//@ngrx/store",
128-
"@npm//rxjs",
129-
],
130-
)
131-
132-
tf_ng_module(
133-
name = "app_test",
134-
testonly = True,
135-
srcs = [
136-
"app_test.ts",
137-
],
138-
deps = [
139-
":app",
140-
":app_state",
141-
":selectors",
142-
"//tensorboard/webapp/angular:expect_angular_core_testing",
143-
"//tensorboard/webapp/angular:expect_ngrx_store_testing",
144-
"//tensorboard/webapp/app_routing:testing",
145-
"//tensorboard/webapp/core/actions",
146-
"@npm//@angular/common",
147-
"@npm//@angular/core",
148-
"@npm//@ngrx/store",
149-
"@npm//@types/jasmine",
150125
],
151126
)
152127

@@ -273,7 +248,6 @@ tf_resource_digest_suffixer(
273248
tf_ng_web_test_suite(
274249
name = "karma_test",
275250
deps = [
276-
":app_test",
277251
"//tensorboard/webapp/alert:test_lib",
278252
"//tensorboard/webapp/alert/store:test_lib",
279253
"//tensorboard/webapp/alert/views:views_test",

‎tensorboard/webapp/app_container.ts

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,6 @@ See the License for the specific language governing permissions and
1313
limitations under the License.
1414
==============================================================================*/
1515
import {Component, ViewContainerRef} from '@angular/core';
16-
import {Store} from '@ngrx/store';
17-
import {filter, take} from 'rxjs/operators';
18-
19-
import {getActiveRoute} from './selectors';
20-
import {coreLoaded} from './core/actions';
21-
import {State} from './app_state';
2216

2317
@Component({
2418
selector: 'tb-webapp',
@@ -29,21 +23,5 @@ export class AppContainer {
2923
// vcRef is required by ngx-color-picker in order for it to place the popup
3024
// in the root node in a modal mode.
3125
// https://github.com/zefoy/ngx-color-picker/blob/94a7c862bb61d7207f21281526fcd94453219b54/projects/lib/src/lib/color-picker.directive.ts#L168-L175
32-
constructor(
33-
private readonly store: Store<State>,
34-
readonly vcRef: ViewContainerRef
35-
) {
36-
// Wait for route to be initialized before dispatching a coreLoaded.
37-
this.store
38-
.select(getActiveRoute)
39-
.pipe(
40-
filter((route) => Boolean(route)),
41-
take(1)
42-
)
43-
.subscribe(() => {
44-
// TODO(stephanwlee): deprecated coreLoaded and use the router actions when all
45-
// apps are using the router.s
46-
this.store.dispatch(coreLoaded());
47-
});
48-
}
26+
constructor(readonly vcRef: ViewContainerRef) {}
4927
}

‎tensorboard/webapp/app_test.ts

Lines changed: 0 additions & 71 deletions
This file was deleted.

‎tensorboard/webapp/core/BUILD

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
load("//tensorboard/defs:defs.bzl", "tf_ng_module")
1+
load("//tensorboard/defs:defs.bzl", "tf_ng_module", "tf_ts_library")
22

33
package(default_visibility = ["//tensorboard:internal"])
44

@@ -19,9 +19,19 @@ tf_ng_module(
1919
],
2020
)
2121

22-
tf_ng_module(
22+
tf_ts_library(
2323
name = "types",
2424
srcs = [
2525
"types.ts",
2626
],
2727
)
28+
29+
tf_ts_library(
30+
name = "state",
31+
srcs = [
32+
"state.ts",
33+
],
34+
deps = [
35+
"//tensorboard/webapp:app_state",
36+
],
37+
)

‎tensorboard/webapp/core/actions/core_actions.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,9 @@ export const pluginUrlHashChanged = createAction(
3838
props<{plugin: PluginId}>()
3939
);
4040

41+
/**
42+
* @deprecated View should not fire coreLoaded to trigger data re-fetch.
43+
*/
4144
export const coreLoaded = createAction('[Core] Loaded');
4245

4346
export const manualReload = createAction('[Core] User Triggered Reload');
@@ -56,6 +59,18 @@ export const pluginsListingFailed = createAction(
5659
props<{failureCode: PluginsListFailureCode}>()
5760
);
5861

62+
export const polymerRunsFetchRequested = createAction(
63+
'[Core] Polymer Component Runs Fetch Requested'
64+
);
65+
66+
export const polymerRunsFetchSucceeded = createAction(
67+
'[Core] Polymer Component Runs Fetch Successful'
68+
);
69+
70+
export const polymerRunsFetchFailed = createAction(
71+
'[Core] Polymer Component Runs Fetch Failed'
72+
);
73+
5974
/**
6075
* Action for when Environment data has been loaded from the WebApp server.
6176
*/

‎tensorboard/webapp/core/core_module.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import {
2525
CORE_STORE_CONFIG_TOKEN,
2626
getConfig,
2727
} from './store/core_initial_state_provider';
28-
import {DeepLinkModule} from '../deeplink/deeplink_module';
2928
import {HashDeepLinker} from '../deeplink';
3029

3130
@NgModule({

‎tensorboard/webapp/core/effects/BUILD

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,11 @@ tf_ng_module(
1111
deps = [
1212
"//tensorboard/webapp:app_state",
1313
"//tensorboard/webapp:tb_polymer_interop_types",
14+
"//tensorboard/webapp/app_routing:types",
15+
"//tensorboard/webapp/app_routing/actions",
16+
"//tensorboard/webapp/app_routing/store",
17+
"//tensorboard/webapp/app_routing/store:types",
18+
"//tensorboard/webapp/core:state",
1419
"//tensorboard/webapp/core:types",
1520
"//tensorboard/webapp/core/actions",
1621
"//tensorboard/webapp/core/store",
@@ -36,6 +41,10 @@ tf_ts_library(
3641
"//tensorboard/webapp:app_state",
3742
"//tensorboard/webapp/angular:expect_angular_core_testing",
3843
"//tensorboard/webapp/angular:expect_ngrx_store_testing",
44+
"//tensorboard/webapp/app_routing:testing",
45+
"//tensorboard/webapp/app_routing:types",
46+
"//tensorboard/webapp/app_routing/actions",
47+
"//tensorboard/webapp/app_routing/store",
3948
"//tensorboard/webapp/core:types",
4049
"//tensorboard/webapp/core/actions",
4150
"//tensorboard/webapp/core/store",

0 commit comments

Comments
 (0)