Chore: Fix an issue with property renaming in polymer #6401

rileyajones · 2023-05-22T22:06:07Z

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

bmd3k

This breaks the boundary between the polymer and angular binaries. This is currently forbidden by policy and an internal test. Is there a way to accomplish this without breaking that boundary?

Is it possible, for example, for the polymer code to define some symbols on the window object ?

tensorboard/webapp/deeplink/hash.ts

bmd3k · 2023-05-26T11:42:31Z

tensorboard/webapp/deeplink/hash.ts

@@ -23,25 +23,28 @@ const TAB = '__tab__';

 @Injectable()
 export class HashDeepLinker implements DeepLinkerInterface {


Can we expand this PR to the point where we are accessing tf_global and tf_storage consistently in the Angular part of the app? Basically get to the point where we can delete TfStorageElement and TfGlobalsElement types. The inconsistency in how we access these types is going to hurt somebody's head in the future.

Alternatively: Perhaps the additional cleanup work can be done in a followup PR is you'd like to keep this PR smaller.

I think I have addressed this, but I'm going to let you resolve this to be sure

tensorboard/components/experimental/plugin_util/plugin_api_host_test.ts

tensorboard/components/tf_globals/globals-polymer.ts

…he binary boundry

…pping is now handles via a map

bmd3k · 2023-11-22T14:41:21Z

tensorboard/webapp/angular_polymer_bridge/BUILD

@@ -0,0 +1,10 @@
+load("//tensorboard/defs:defs.bzl", "tf_ts_library")


We already have tb_polymer_interop_types. No need to redeclare the abstractions here.

I was trying to keep functionality out of a "types" file, but thinking about it more, that does make sense

bmd3k · 2023-11-22T14:41:57Z

tensorboard/webapp/deeplink/hash.ts


    // 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();
+    getGlobals('setUseHash')?.(true);


There should be minimal or no changes to this file, as long as the polymer binary properly attaches the api to the TFGlobals object.

tensorboard/components/tf_globals/globals-polymer.ts

bmd3k · 2023-11-27T18:59:00Z

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

@@ -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']?.();


Whatever you're doing here is probably not going to impress the readability team. You should be able to continue to rely on proper JS/TS objects in the polymer code. (and elsewhere in the polymer code).

With property renaming enabled the two binaries will check different properties. Something akin to this is necessary to enable them to communicate.

I can definitely see why the readability team wouldn't love this though

bmd3k · 2023-11-27T19:02:10Z

tensorboard/webapp/deeplink/hash.ts

    // 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']?.();


If you properly declared the API interface for the Angular binary then shouldn't this be tensorboard.tf_storage.migrateLegacyURLScheme?

It should be unnecessary to access the API like this (and I don't think it'll impress the readability team).

With property renaming enabled the polymer binary will not store this as window.tensorboard.tf_storage.migrateLegacyURLScheme, it will be something like window.AB.CD.EF, then the angular binary will attempt to check a different property, say window.CD.DE.FG. I found string indexes to be the cleanest way to ensure the same properties are accessed.

Spoke offline. Riley to look into how to fix this, possibly using typings, possibly using some internal examples of interactions with thirdparty js binaries as motivation.

This was tricky to solve given that both property renaming and definition files work differently internally and externally. Fortunately I did figure it out with the only compromise being the need to declare window.tensorboard in both the definitions file and the types file.

bmd3k · 2023-12-05T20:58:22Z

tensorboard/components/tf_globals/globals-polymer.ts

-  override _template = null;
-  tf_globals = tf_globals;
-}
+window.tensorboard.tf_globals = tf_globals;


Cool, it does seem to work. This is how it looks with property renaming turned on:

tensorboard/components/tf_globals/globals-polymer.ts

tensorboard/webapp/tb_polymer_interop_types.ts

tensorboard/components/tf_globals/globals-polymer.ts

bmd3k · 2023-12-06T15:02:49Z

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

@@ -15,12 +15,12 @@ limitations under the License.
 /**
 * Implements core plugin APIs.
 */
+import '../../../webapp/tb_polymer_interop_types';


PR description hygiene: You include a couple screenshots that IMO are not very valuable. I think you're trying to tell me how you tested your changes but you could probably just use words for that.

A more valuable screenshot would be one that actually shows how the new code is resilient to variable renaming (perhaps a similar screenshot to the one I posted where you see how window.tensorboard.tf_globals appears in the console).

Prior to my changes those pages never loaded because an error was thrown. I was attempting to highlight that the page at least loads now.
I definitely see your point though so I've updated my description to just include a screenshot of the console

I don't understand. TimeSeries and Scalars pages have always loaded successfully. (except for the occasional P0 outage, I guess).

It didn't load with property renaming enabled

tensorboard/webapp/widgets/histogram/BUILD

bmd3k · 2023-12-06T15:42:21Z

tensorboard/webapp/tb_polymer_interop_types.ts


-export declare interface TfGlobalsElement extends HTMLElement {
-  tf_globals: TfGlobals;
+  interface Window {


Is this redundant with tb_polymer_interop_type_definitions?

For some reason the polymer binary doesn't recognize the global definition (this is why it needs to import tb_polymer_interop_types). That means that without this definition here the polymer binary doesn't recognize that this type exists.

I patched in your PR locally and deleted this interface Window block. Everything seems to compile and run. I didn't try importing it internally, though. If you still think there is some reason to duplicate it here, best leave a comment.

I take it you made the change to the OSS project? This works fine there but internally you'll see this error
https://screenshot.googleplex.com/8A2L2M3shUuMu29

I'll add a comment

tensorboard/webapp/tb_polymer_interop_type_definitions.d.ts

tensorboard/webapp/deeplink/hash_test.ts

tensorboard/webapp/deeplink/hash.ts

bmd3k

The changes look very good now. Thanks for persisting at it.

Would you consider doing this for any remaining Angular/Polymer interop types? Would that be enough to turn on property renaming?

bmd3k · 2023-12-07T03:15:46Z

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

@@ -15,12 +15,12 @@ limitations under the License.
 /**
 * Implements core plugin APIs.
 */
+import '../../../webapp/tb_polymer_interop_types';


I don't understand. TimeSeries and Scalars pages have always loaded successfully. (except for the occasional P0 outage, I guess).

tensorboard/webapp/deeplink/hash.ts

tensorboard/webapp/deeplink/hash_test.ts

bmd3k · 2023-12-11T12:11:02Z

tensorboard/webapp/tb_polymer_interop_types.ts


-export declare interface TfGlobalsElement extends HTMLElement {
-  tf_globals: TfGlobals;
+  interface Window {


I patched in your PR locally and deleted this interface Window block. Everything seems to compile and run. I didn't try importing it internally, though. If you still think there is some reason to duplicate it here, best leave a comment.

rileyajones · 2023-12-12T01:10:55Z

The changes look very good now. Thanks for persisting at it.

Would you consider doing this for any remaining Angular/Polymer interop types? Would that be enough to turn on property renaming?

It would be nice to get this fully working. As far as I can tell the main issue remaining after this is with some of the dependencies used by the polymer binary. Specifically dagre and THREE. That could be solvable and might make for an interesting on duty task.

rileyajones requested a review from bmd3k May 22, 2023 22:06

bmd3k reviewed May 23, 2023

View reviewed changes

rileyajones requested a review from bmd3k May 25, 2023 18:38

rileyajones marked this pull request as ready for review May 25, 2023 18:38

bmd3k reviewed May 26, 2023

View reviewed changes

rileyajones force-pushed the ts-readability branch from 6bd54ab to a7c794c Compare July 17, 2023 22:49

rileyajones requested a review from bmd3k July 19, 2023 17:40

bmd3k reviewed Jul 20, 2023

View reviewed changes

rileyajones added 7 commits November 21, 2023 18:03

fix an issue with property renaming in polymer

263adb6

update tf-storage and tf-globals to use window to pass data accross t…

7ddf2ee

…he binary boundry

use global type

035d27e

remove references to older tf storage and tf globals sharing

03dc82e

fix broken test

c7933b9

renaming

eda4702

add a dependency to each binary which handles the communication, rema…

a7e249f

…pping is now handles via a map

rileyajones force-pushed the ts-readability branch from 818c6e5 to a7e249f Compare November 21, 2023 19:31

rileyajones added 3 commits November 21, 2023 19:34

add license

65c04f3

update typing in test

6a8e864

revert changes to interop types

6e7f63d

rileyajones requested a review from bmd3k November 21, 2023 22:43

bmd3k reviewed Nov 22, 2023

View reviewed changes

rileyajones added 2 commits November 22, 2023 19:47

re-simplify things...

9d69301

remove unneeded custom elements

f56620b

rileyajones requested a review from bmd3k November 22, 2023 20:55

bmd3k reviewed Nov 27, 2023

View reviewed changes

rileyajones requested a review from bmd3k November 27, 2023 19:51

rileyajones added 2 commits November 28, 2023 00:24

change to using a definitions file over string indexing

95edb20

remove unneeded cast and revert unnecessary change

0ece34a

bmd3k reviewed Dec 6, 2023

View reviewed changes

address some pr comments

7c80cac

rileyajones added 3 commits December 6, 2023 17:39

Merge branch 'master' into ts-readability

3f107c2

finish addressing pr comments

ef643f3

fix typing in tests

6066960

rileyajones requested a review from bmd3k December 6, 2023 23:56

bmd3k approved these changes Dec 11, 2023

View reviewed changes

address final pr comments

e42f2d0

rileyajones merged commit d0c5860 into tensorflow:master Dec 15, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Fix an issue with property renaming in polymer #6401

Chore: Fix an issue with property renaming in polymer #6401

rileyajones commented May 22, 2023 •

edited

Loading

bmd3k left a comment

bmd3k May 26, 2023

rileyajones Nov 30, 2023

bmd3k Nov 22, 2023 •

edited

Loading

rileyajones Nov 22, 2023

bmd3k Nov 22, 2023

bmd3k Nov 27, 2023

rileyajones Nov 27, 2023

rileyajones Nov 27, 2023

bmd3k Nov 27, 2023

rileyajones Nov 27, 2023

bmd3k Nov 27, 2023

rileyajones Nov 28, 2023

bmd3k Dec 5, 2023

bmd3k Dec 6, 2023

rileyajones Dec 6, 2023

bmd3k Dec 7, 2023

rileyajones Dec 12, 2023

bmd3k Dec 6, 2023

rileyajones Dec 6, 2023

bmd3k Dec 11, 2023

rileyajones Dec 12, 2023

bmd3k left a comment

bmd3k Dec 7, 2023

bmd3k Dec 11, 2023

rileyajones commented Dec 12, 2023

		@@ -23,25 +23,28 @@ const TAB = '__tab__';

		@Injectable()
		export class HashDeepLinker implements DeepLinkerInterface {

		@@ -0,0 +1,10 @@
		load("//tensorboard/defs:defs.bzl", "tf_ts_library")

Chore: Fix an issue with property renaming in polymer #6401

Chore: Fix an issue with property renaming in polymer #6401

Conversation

rileyajones commented May 22, 2023 • edited Loading

Motivation for features / changes

Technical description of changes

Screenshots of UI changes (or N/A)

bmd3k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmd3k Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmd3k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileyajones commented Dec 12, 2023

rileyajones commented May 22, 2023 •

edited

Loading

bmd3k Nov 22, 2023 •

edited

Loading