Skip to content

Commit

Permalink
Chore: Fix an issue with property renaming in polymer (#6401)
Browse files Browse the repository at this point in the history
## Motivation for features / changes
We keep the property renaming compiler flag turned off because of the
many issues it causes with polymer. This should bring us one step closer
towards enabling it.
b/281726974

Googlers, see that this imports correctly cl/585788456

## Technical description of changes
The hack of creating custom elements and then referencing properties on
them doesn't really work with property renaming turned on because the
portions of the code which reference the properties are often not being
changed. To get around this while still maintaining the binary
seperation I am updating the code to write properties to `window`

## Screenshots of UI changes (or N/A)
Properties are not being renamed

![image](https://github.com/tensorflow/tensorboard/assets/78179109/863f5cd7-4805-4e8f-bf82-309d3dd51716)
  • Loading branch information
rileyajones authored Dec 15, 2023
1 parent 6c06cc5 commit d0c5860
Show file tree
Hide file tree
Showing 14 changed files with 96 additions and 88 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@ limitations under the License.
/**
* Implements core plugin APIs.
*/
import '../../../webapp/tb_polymer_interop_types';
import {Injectable} from '@angular/core';
import {Store} from '@ngrx/store';
import {distinctUntilChanged, filter} from 'rxjs/operators';
import {State} from '../../../webapp/app_state';
import {getAppLastLoadedTimeInMs} from '../../../webapp/selectors';
import {TfStorageElement} from '../../../webapp/tb_polymer_interop_types';
import {MessageId} from './message_types';
import {Ipc} from './plugin-host-ipc';

Expand All @@ -32,7 +32,6 @@ export class PluginCoreApiHostImpl {
) {}

init() {
const tfStorage: TfStorageElement = document.createElement('tf-storage');
this.ipc.listen(MessageId.GET_URL_DATA, (context) => {
if (!context) {
return;
Expand All @@ -41,7 +40,8 @@ export class PluginCoreApiHostImpl {
const result: {
[key: string]: string;
} = {};
const urlDict = tfStorage.tf_storage.getUrlHashDict();

const urlDict = window.tensorboard.tf_storage.getUrlHashDict();
for (let key in urlDict) {
if (key.startsWith(prefix)) {
const pluginKey = key.substring(prefix.length);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
getExperimentIdsFromRoute,
getRuns,
} from '../../../webapp/selectors';
import {TfStorageElement} from '../../../webapp/tb_polymer_interop_types';
import {provideMockTbStore} from '../../../webapp/testing/utils';
import {PluginCoreApiHostImpl} from './core-host-impl';
import {MessageId} from './message_types';
Expand Down Expand Up @@ -253,8 +252,8 @@ describe('plugin_api_host test', () => {

it('returns url data from the tf storage', () => {
// Do not rely on Polymer bundle in the test.
const createElementSpy = spyOn(document, 'createElement');
createElementSpy.withArgs('tf-storage').and.returnValue({
window.tensorboard = {
tf_globals: {},
tf_storage: {
getUrlHashDict: () => {
return {
Expand All @@ -265,8 +264,7 @@ describe('plugin_api_host test', () => {
};
},
},
} as unknown as TfStorageElement);

} as any;
coreApi.init();
const actual = triggerGetUrlData({pluginName: 'plugin_id'});
expect(actual).toEqual({
Expand Down
1 change: 1 addition & 0 deletions tensorboard/components/tf_globals/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ tf_ts_library(
],
deps = [
":tf_globals_lib",
"//tensorboard/webapp:tb_polymer_interop_types",
"@npm//@polymer/decorators",
"@npm//@polymer/polymer",
],
Expand Down
18 changes: 11 additions & 7 deletions tensorboard/components/tf_globals/globals-polymer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,16 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {customElement} from '@polymer/decorators';
import {PolymerElement} from '@polymer/polymer';
import '../../webapp/tb_polymer_interop_types';
import * as tf_globals from './globals';

@customElement('tf-globals')
class TfGlobals extends PolymerElement {
override _template = null;
tf_globals = tf_globals;
}
/**
* Attach API to window for interoperability with the Angular binary.
* The full shared type is defined in tensorboard/webapp/tb_polymer_interop_type_definitions.d.ts
* Defining it this way doesn't matter too much while property renaming is turned off,
* but at some point in the future we would like to enable it.
*/
window.tensorboard = {
...window.tensorboard,
tf_globals,
};
1 change: 1 addition & 0 deletions tensorboard/components/tf_storage/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ tf_ts_library(
strict_checks = False,
deps = [
":tf_storage_lib",
"//tensorboard/webapp:tb_polymer_interop_types",
"@npm//@polymer/decorators",
"@npm//@polymer/polymer",
"@npm//@types/lodash",
Expand Down
18 changes: 11 additions & 7 deletions tensorboard/components/tf_storage/tf-storage-polymer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,16 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {customElement} from '@polymer/decorators';
import {PolymerElement} from '@polymer/polymer';
import '../../webapp/tb_polymer_interop_types';
import * as tf_storage from './index';

@customElement('tf-storage')
class TfStorage extends PolymerElement {
override _template = null;
tf_storage = tf_storage;
}
/**
* Attach API to window for interoperability with the Angular binary.
* The full shared type is defined in tensorboard/webapp/tb_polymer_interop_type_definitions.d.ts
* Defining it this way doesn't matter too much while property renaming is turned off,
* but at some point in the future we would like to enable it.
*/
window.tensorboard = {
...window.tensorboard,
tf_storage,
};
4 changes: 4 additions & 0 deletions tensorboard/webapp/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,12 @@ tf_ng_module(
tf_ts_library(
name = "tb_polymer_interop_types",
srcs = [
"tb_polymer_interop_type_definitions.d.ts",
"tb_polymer_interop_types.ts",
],
deps = [
# This build target is shared with the polymer binary, do not
# add any more dependencies.
"//tensorboard/webapp/feature_flag:types",
],
)
Expand Down Expand Up @@ -107,6 +110,7 @@ tf_ng_module(
":oss_plugins_module",
":selectors",
":store_module",
":tb_polymer_interop_types",
"//tensorboard/components/experimental/plugin_util:plugin_host",
"//tensorboard/webapp/alert",
"//tensorboard/webapp/alert/views:alert_snackbar",
Expand Down
1 change: 0 additions & 1 deletion tensorboard/webapp/core/effects/core_effects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import {
} from '../../app_routing/store/app_routing_selectors';
import {RouteKind} from '../../app_routing/types';
import {getEnabledExperimentalPlugins} from '../../feature_flag/store/feature_flag_selectors';
import '../../tb_polymer_interop_types';
import {DataLoadState} from '../../types/data';
import {
TBServerDataSource,
Expand Down
1 change: 1 addition & 0 deletions tensorboard/webapp/deeplink/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ tf_ng_module(
],
deps = [
":deeplink",
"//tensorboard/webapp:tb_polymer_interop_types",
"//tensorboard/webapp/angular:expect_angular_core_testing",
"@npm//@types/jasmine",
],
Expand Down
10 changes: 4 additions & 6 deletions tensorboard/webapp/deeplink/deeplink_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import '../tb_polymer_interop_types';
import {TestBed} from '@angular/core/testing';
import {HashDeepLinker, TEST_ONLY} from './hash';

Expand All @@ -36,20 +37,17 @@ describe('deeplink', () => {
// to not make use of the hash (it does).

// Do not rely on Polymer bundle in the test.
const createElementSpy = spyOn(document, 'createElement');
createElementSpy.withArgs('tf-storage').and.returnValue({
window.tensorboard = {
tf_storage: {
setString: setStringSpy,
getString: getStringSpy,
migrateLegacyURLScheme: migrateLegacyURLSchemeSpy,
getUrlHashDict: () => ({}),
},
} as any);
createElementSpy.withArgs('tf-globals').and.returnValue({
tf_globals: {
setUseHash: setUseHashSpy,
},
} as any);
createElementSpy.and.callThrough();
};

deepLinker = TestBed.inject(HashDeepLinker);
});
Expand Down
18 changes: 6 additions & 12 deletions tensorboard/webapp/deeplink/hash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/

import '../tb_polymer_interop_types';
import {Injectable} from '@angular/core';
import {TfStorageElement} from '../tb_polymer_interop_types';
import {DeepLinkerInterface, SetStringOption} from './types';

// TODO(tensorboard-team): merge this module with tf_storage/storage.ts when
Expand All @@ -23,29 +22,24 @@ const TAB = '__tab__';

@Injectable()
export class HashDeepLinker implements DeepLinkerInterface {
private readonly tfStorage: TfStorageElement;

constructor() {
this.tfStorage = document.createElement('tf-storage');
const tfGlobals = document.createElement('tf-globals');

// Note: `migrateLegacyURLScheme()` must be called before `setUseHash`, so
// that tfStorage reads from the actual URL, not the fake hash for tests
// only.
tfGlobals.tf_globals.setUseHash(true);
this.tfStorage.tf_storage.migrateLegacyURLScheme();
window.tensorboard.tf_storage.migrateLegacyURLScheme();
window.tensorboard.tf_globals.setUseHash(true);
}

getString(key: string): string {
return this.tfStorage.tf_storage.getString(key);
return window.tensorboard.tf_storage.getString(key);
}

setString(key: string, value: string, options?: SetStringOption): void {
this.tfStorage.tf_storage.setString(key, value, options);
window.tensorboard.tf_storage.setString(key, value, options);
}

getPluginId(): string {
return this.getString(TAB);
return window.tensorboard.tf_storage.getString(TAB);
}

setPluginId(pluginId: string, options?: SetStringOption): void {
Expand Down
18 changes: 0 additions & 18 deletions tensorboard/webapp/deeplink/hash_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,24 +32,6 @@ describe('hash storage test', () => {

setStringSpy = jasmine.createSpy();
getStringSpy = jasmine.createSpy();

// Cannot safely stub out window.location.hash or rely on test framework
// to not make use of the hash (it does).

// Do not rely on Polymer bundle in the test.
const createElement = spyOn(document, 'createElement');
createElement.withArgs('tf-storage').and.returnValue({
tf_storage: {
setString: setStringSpy,
getString: getStringSpy,
},
} as any);
createElement.withArgs('tf-globals').and.returnValue({
tf_globals: {
setUseHash: jasmine.createSpy(),
},
} as any);
createElement.and.callThrough();
});

afterEach(() => {
Expand Down
42 changes: 42 additions & 0 deletions tensorboard/webapp/tb_polymer_interop_type_definitions.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/* Copyright 2023 The TensorFlow Authors. All Rights Reserved.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
declare global {
interface Window {
tensorboard: {
tf_storage: TfStorage;
tf_globals: TfGlobals;
};
}
}

export interface SetStringOption {
defaultValue?: string;
/**
* When true, setting the string does not push a new state onto the history.
* i.e., it uses `history.replaceState` instead of `history.pushState`.
*/
useLocationReplace?: boolean;
}

export interface TfStorage {
setString(key: string, value: string, options?: SetStringOption): void;
getString(key: string): string;
migrateLegacyURLScheme(): void;
getUrlHashDict(): Record<string, string>;
}

export interface TfGlobals {
setUseHash(use: boolean): void;
}
38 changes: 9 additions & 29 deletions tensorboard/webapp/tb_polymer_interop_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,34 +13,25 @@ See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {FeatureFlags} from './feature_flag/types';
import {TfGlobals, TfStorage} from './tb_polymer_interop_type_definitions';

declare global {
// createElement type uses the TagNameMap underneath and returns the right type.
interface HTMLElementTagNameMap {
'tf-backend': TfBackendElement;
'tf-globals': TfGlobalsElement;
'tf-feature-flags': TfFeatureFlagsElement;
'tf-storage': TfStorageElement;
'tf-paginated-view-store': TfPaginatedViewStoreElement;
'vz-histogram-timeseries': VzHistogramTimeSeriesElement;
}
}

export declare interface TfGlobals {
setUseHash(use: boolean): void;
}

export declare interface TfGlobalsElement extends HTMLElement {
tf_globals: TfGlobals;
}

export declare interface SetStringOption {
defaultValue?: string;
/**
* When true, setting the string does not push a new state onto the history.
* i.e., it uses `history.replaceState` instead of `history.pushState`.
*/
useLocationReplace?: boolean;
// This type needs to be redeclared here due to an inconsistency with the
// internal and external compilers.
interface Window {
tensorboard: {
tf_storage: TfStorage;
tf_globals: TfGlobals;
};
}
}

export declare interface TfFeatureFlags {
Expand All @@ -54,17 +45,6 @@ export declare interface TfFeatureFlagsElement extends HTMLElement {
tf_feature_flags: TfFeatureFlags;
}

export declare interface TfStorage {
setString(key: string, value: string, options?: SetStringOption): void;
getString(key: string): string;
migrateLegacyURLScheme(): void;
getUrlHashDict(): Record<string, string>;
}

export declare interface TfStorageElement extends HTMLElement {
tf_storage: TfStorage;
}

export declare interface Store {
refresh(): Promise<void>;
}
Expand Down

0 comments on commit d0c5860

Please sign in to comment.