-
Notifications
You must be signed in to change notification settings - Fork 1.7k
core: upgrade Polymer to v2.7 #2392
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
Conversation
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.
nfelt
left a comment
There was a problem hiding this 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.
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 |
|
My understanding was that we planned for the commit message to include |
|
@wchargin good idea. I was going to do that when it was time for squash but that sounds better. It is done! |
wchargin
left a comment
There was a problem hiding this 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.
|
I was able to get the internal sync to work. I think we are ready to merge this soon. |
|
Note that this PR has |
nfelt
left a comment
There was a problem hiding this 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 :)
🎉 🎉 🎉
wchargin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 indeed!
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