Skip to content

Conversation

@stephanwlee
Copy link
Contributor

@stephanwlee stephanwlee commented Jun 27, 2019

  • Motivation for features / changes
    Polymer 1 is quite old and we should migrate off to 2 and possibly to 3.
    This is partially for its internal deprecation but also to revamp build tooling:
    we are currently tied to a process called Vulcanization because we have to
    build HTML files.

  • Detailed steps to verify changes work correctly (as executed by you)
    Ran/clicked/exercised all plugins.

  • Warning
    Although we tried to thorough in our testing, there may be few visual/functional
    regressions. We intend to fix forward those bugs as they are identified.

  • Merge plan: squash

  • Commit Message

core: update the dependencies with Polymer v2 (#2231)                           
core: bump up JSCompiler langout to ES2015 (#2231)                              
core: use v2 Polymer API (#2231)                                                
core: fixed Plottable CSS issue (#2231)                                         
core: <style> should appear inside template (#2231)                             
cleanup: Replaced paper-menu with paper-listbox (#2231)                         
core: fixed the limit setter (#2231)                                                                                                                                                                                                          
graph: fixed most visual regressions of tf-graph (#2251)                        
graph: fixed tf-graph-icons and moved it for reusability (#2251)                
graph: fix graph node toggle bug (#2251)                                        
graph: fix graph fitting in tf-graph-controls (#2251)                           
graph: Fixed input trace (#2251)                                                
graph: fixed broken context menu (#2251)                                        
graph: fixed info panel buttons (#2251)                                         
graph: fixed remaining global selector (#2251)                                  
core: fixed the Plottable interaction (#2249)                                   
profile: fixed mouse interaction (#2286)                                        
profile: fixed CSS issues (#2286)                                               
profile: fixed hosts not updating issue (#2286)                                 
profile: fix misc issues (#2286)                                                
profile: overview and pipline view fixes (#2286)                                
core: fix the line chart tooltip (#2277)                                        
core: fix the data-loader behavior (#2250)                                      
core: use proper CSS selector syntax for :host (#2250)                          
core: fix dashboard-style slot usage to property "slot" (#2250)                 
cpv: fixed custom templatizer in paginated-view2 (#2273)                        
cpv: Restructure files (#2273)                                                  
cpv: Implement category-paginated-view (#2273)                                  
cpv: Migrate to tf-category-paginated-view (#2273)                              
cpv: Fix test for tf-category-paginated-view (#2273)                            
cpv: Remove unused paginated-view and renamed test (#2273)                      
hparams: fixed CSS and initialization (#2341)                                   
debugger: style fixes (#2341)                                                   
debugger: now works as expected (#2341)                                         
infra: compile TypeScript to ES6 (#2341)                                        
projector: fixes for Polymer 2 (#2341)                                          
what-if: fixes for Polymer 2 (#2341)                                            
image: fix the loader and pagination (#2341)                                    
custom scalars: fixed chart rendering (#2341)                                   
mesh: fix for Polymer 2 (#2341)                                                 
cleanup: removed extreanous logs on tooltip (#2372)                             
test: update webcomponent references in test files (#2343)                      
test: fixed the tf-category-paginated-view (#2343)                              
pr-curve: fix the basic functionalities (#2370)                                 
test: unbreak all the tests (#2396)                                             
core: create a tf polymer lib for polymer deps  (#2396)                         
fixed standalone projector demo (#2396)                                         
core: fix the mirror url (#2402)
core: adopt the Polymer 2 externs (#2419)
  • WANT_LGTM=all

JSCompiler turns Object.create into ES5 compatible way which causes
HTMLElement to throw. More specifically, Polymer uses Object.create to
create a subclass of HTMLElement but JSC turns it into
HTMLElement.call(this) which causes browser runtime to throw.

Object.create was introduced to main stream as part of ES2015.
- content -> slot
  - Used grep to look for sidebar, center, dropdown-content which are
    the common slot usage in TensorBoard.
- customStyle -> polymerEl.updateStyles
- ::content -> ::slotted(selector)
- this.querySelector -> this.root.querySelector
  - Note that not all usage of querySelector are fixed. They will be
    fixed over subsequent changes.
In order for Plottable based charts to properly work, we need to load
the plottable.css. Before, when using shadyCSS, including plottable.css
in the plottable.html was sufficient but now that we have the real
shadow CSS, we need to embed the CSS (styles) in respective components
using `<style include="componentName"></style>`.
Polymer2 now enforces that this is true.
With Polymer2, paper-menu is deprecated in favor of paper-listbox.
Prior to the change, TensorBoard was using paperInput.valueAsNumber
property which does not seem to exist in the newer version. Removed it.
Nature of the changes: with Polymer upgrade, we now use the real
shadowDOM and our app broke because of that. Some of the changes are:
1. Incorrect selector: instead of global/element.querySelector, we now
use the proper Polymer API or use querySelector on a shadowRoot.
2. CSS changes: "::content" is deprecated.
3. CSS stylesheet is mostly sandboxed and changed document.styleSheets
to copy only the necessary portions.

Functional break that was not addressed:
1. node toggle is broken
2. fit to screen is broken
3. tf-graph-controls' legend is not rendering correctly.
4. icons (for the minimap, legends, info panel) are broken

In subsequent changes, we will address them.
In the polyfilled WebComponent world where no DOM is properly scoped
allowed SVG definitions to be declared in one part of the DOM and reused
anywhere. Using this property, tf-graph-scene declared the SVG
definitions for nodes and edges while other components like legend,
info-panel, and the sidebar were using them.

Now that we can no longer do that, we decided to create a component used
by most of above components that define the gradient/node/edges where it
is used. This does create many duplicate of the definitions but does not
seem to have negative impact on runtime performance.
tf-graph-controls was using global querySelector which does not
work as expected with WebComponents. This code was especially less ideal
since tf-graph-controls does not render tf-graph-scene.

Addressed the issue by bubbling up user's action to a component that
renders the tf-graph-scene and created public fit method for all
components that are in between tf-graph-dashboard and tf-graph-scene.
Few problems:
1. listener broke
2. traceInput method was doing a global querySelector

Solution:
Instead of mutating the DOM in the tf-graph-controls, in this change, we
decided to just propagate that information to the graph rendering
component, tf-graph-scene. tf-graph-scene now using that information
makes necessary DOM modification.
Problem: used global query selector to find ".context-menu" in the
entire document.

Solution: graph scene now provides an API for accessing the DOM useful
for rendering the context-menu.
Problem: info panel was firing event to a sibling component which is
obtain via global querySelector
Solution: now tf-graph supports API for add/removing and group/ungroup
a node from the renderHierachy. The tf-node-info simply communicate
user's intent via event which tf-graph-board listens to and makes
appropriate API calls to the tf-graph.
Plottable interactions are broken with the WebComponents due to its
event delegation and relative position computation -- their DOM
traversal was not compatible with the WebComponents. More information at
palantir/plottable#3350.

Because of visibility (private and protected), it was not viable to
subclass the interactions to fix the issue so we have decided to monkey
patch the broken functions in JavaScript. In farther future, we may want
to use raw DOM events to handle the interactions instead of using the
Plottable abstraction.
Profile plugin was using Plottable interaction to handle mouse events.
Because of Plottable's known issues with eventing and web components, we
now use browser APIs to handle the trivial event.
1. info pane was programmatically colored but it no longer works
2. "hidden" attribute did not behave as expected
When active hosts update, due to the discrepency with v1 version of
paper-dropdown-menu and paper-listbox, the dropdown does not react to
any changes in the listbox DOM/values changes.

To be honest here, I expect more part of the apps to be broke with this
behavioral change. We will fix them one by one.
1. Removed deprecated paper-menu
2. style tag should appear inside template
3. missing/wrong dependencies
4. usage of prop before it is instantiated
5. prevented double chart rendering for input-pipe-analyzer
Line chart took a bit different approach in terms of tooltip: because we
showed large content, we wanted it to look reasonable in whichever
orientation appearing above the sidebar using a technical called portal.

The way we portaled previously in Polymer1 no longer works in Polymer2
especially because how real shadow CSS works. Unlike ShadyCSS where it
populated global style tag that is accessible anywhere in the DOM tree,
now, the styles are properly encapsulated inside a component. As a
result, styling for the vz-chart-tooltip became quite difficult.

With this change, to get around the styling issue, the vz-chart-tooltip
now take the name of the custom element and instantiate the element
(of course, with its style defined inside) and attached it to the body
of the document. When doing it, the vz-chart-tooltip style it a bit.
Cause:
1. Lifecycle methods have changed and component gets attached to DOM
even if it is used conditionally with dom-if. Specifically, behavior of
`attached` has changed.
2. Child components can no longer reason and know to rerender the chart
as restamp almost became meaningless. We now use, at the dashboard
level, decide to set active state of the card based on tf-category-pane
and state of the tf-paginated-view.
As per https://developer.mozilla.org/en-US/docs/Web/CSS/:host(), we have
been using incorrect/non-standard syntax for defining a selector on
:host. This change addresses that.
Polymer 2 conforms to the WebComponent standard and now only supports
usage of slot with the "slot" property instead of the classname.

TensorBoard's dashboard-style has been styling a slot using className
which no longer works.
This now conforms to the new API.
- tf-paginated-view2 -> tf-dom-repeat
- tf-category-pane -> tf-category-paginated-view
and fixed histogram with the new category-paginated-view.

This change improves the tf-paginated-view to work around the issue.
Currently tf-category-pane and tf-paginated-view together determine
whether a child is visible or "active". The problem is that in canonical
polymer component structure, it is difficult to pass information
"downwards" when the common ancestor is a host component. Without an
easy way, the proper way is for the host component,
tf-histogram-dashboard to collect the information and pass it down to
the respective tf-histogram-loader. This pattern was implemented but
caused nontrivial bloat that had to be repeated everywhere that used
tf-category-pane and tf-paginated-view. There are rough two ways to
solve this issue: (1) mixin and (2) dom-repeat like component.

We decided to use (2) by improving our tf-paginated-view2 experiment.
In the process, we factored out our custom template implementation as a
mixin (otherwise the component became very overwhelming) to separate out
different concerns.
This change migrates usage of category-pane and paginated-view to
the new tf-category-paginated-view.
@stephanwlee stephanwlee changed the title core: upgrade Polmyer from 1 to 2. core: upgrade Polmyer to v2.7 Jun 28, 2019
@stephanwlee stephanwlee requested review from nfelt and wchargin June 28, 2019 23:28
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

🎉 Hooray! Thanks so much for pulling this off :)

LGTM from me, assuming we've confirmed sync+build internally is okay with this change?

Let's get an LGTM from William too for such a mega change.

@stephanwlee
Copy link
Contributor Author

sync+build internally is okay with this change?

I was going to do this after the merge but, with your comment, I wanted to take a look at what kind of changes would be required. I tried to follow tomservo process to do so except I noticed something weird starting from b5f4507. We are currently syncing JavaScript files in bazel-genfiles built by bazel --batch build but, starting from that commit, I cannot make the Vulcanization put JavaScript in that directory. I tried to see how Tomservo continues to work, with the bazel behavioral change, but I am failing to logically make sense. My current hypothesis is that bazel server running on tomservo is stuck on 0.19 and everything magically is working.

@wchargin wchargin changed the title core: upgrade Polmyer to v2.7 core: upgrade Polymer to v2.7 Jul 1, 2019
@wchargin
Copy link
Contributor

wchargin commented Jul 1, 2019

My understanding was that we planned for the commit message to include
pointers to previous pull requests of interest. If that’s still the
case, could you please include a preview of that message in the PR
description? (You may find git shortlog origin/master..origin/polymer2
useful as a starting point, though it may have too many entries and may
not include all the PR numbers.)

@stephanwlee
Copy link
Contributor Author

@wchargin good idea. I was going to do that when it was time for squash but that sounds better. It is done!

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Sent you a note on internal chat regarding sync.

Polymer now ships proper Closure externs file and hence we would
like to update our extern (used to be from closure repo).

Few things to note:
1. separate build target for externs (to mirror internal)
2. refer to those externs using a build target and with our new `jscomp-externs` attribute.
3. suppress the polymer/externs/webcomponents_externs.js type error.
@stephanwlee stephanwlee requested review from nfelt and wchargin July 16, 2019 15:33
@stephanwlee
Copy link
Contributor Author

I was able to get the internal sync to work. I think we are ready to merge this soon.
For more information, please refer to cl/256261243.

@stephanwlee
Copy link
Contributor Author

Note that this PR has WANT_LGTM=all :D

Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

LGTM since we have a plan for the internal sync now :)

🎉 🎉 🎉

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

🎉 indeed!

@stephanwlee stephanwlee merged commit fa56d0c into master Jul 18, 2019
@stephanwlee stephanwlee deleted the polymer2 branch July 18, 2019 18:28
@wchargin wchargin added the polymer2 Visual or functional regressions introduced by the Polymer 2 upgrade that should be fixed by TF 2.0. label Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

polymer2 Visual or functional regressions introduced by the Polymer 2 upgrade that should be fixed by TF 2.0.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants