Skip to content
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

Upgrade to Polymer 2.0 #5

Closed
18 tasks done
justincy opened this issue Mar 9, 2017 · 19 comments
Closed
18 tasks done

Upgrade to Polymer 2.0 #5

justincy opened this issue Mar 9, 2017 · 19 comments

Comments

@justincy
Copy link
Member

justincy commented Mar 9, 2017

Polymer 2.0 RC was released this week. Lets work on upgrading the components to 2.0.

  • fs-behavior
  • fs-client
  • fs-pedigree
  • fs-person-behavior
  • fs-person-card
  • fs-person-chip
  • fs-person-facts
  • fs-person-families
  • fs-person-portrait
  • fs-person-sources
  • fs-person-summary
  • fs-signin
  • sample app
    • fix fs-client ordering
    • fix toolbar header
    • fix person-history menu so that the dropdown display
    • fix person-history menu so that it updates the label reflecting the currently selected value
    • fix auth detection (likely related to fs-client changes
@justincy
Copy link
Member Author

justincy commented Mar 9, 2017

I think we'll start with fs-signin and then fs-person-portrait and it's dependencies. Everything should be pretty straightforward after that.

@justincy
Copy link
Member Author

I have fs-signin, fs-client, and fs-behavior upgraded to Polymer 2. I'm going for legacy mode which lets us continue using the 1.0 API with minimal changes; only changes I've done are related to CSS shims. All changes are being committed to polymer-2 branches in each repo.

@justincy
Copy link
Member Author

While working on the upgrade for fs-person-families I hit a problem with timing of lifecycle events. In Polymer 1.3+ property observers were guaranteed to not fire until after ready(). That assumption is currently broken in Polymer 2 hybrid mode and is breaking dynamically added elements because they're trying to fetch data from the API before their embedded fs-client instance is setup so undefined errors are being thrown.

@justincy
Copy link
Member Author

This jsbin seems to say that the timing I relied on wasn't guaranteed: https://jsbin.com/bewaquwugo/edit?html,console,output I'm seeing observers being called before ready() in 1.x

@justincy
Copy link
Member Author

justincy commented Mar 16, 2017

Turns out the issue isn't with timing of property observers with relation to ready() being called. The problem is due to the order of parents and children initialization changing. In 1.x the ready() methods of all children were called before parents but in v2 that isn't happening.

@justincy
Copy link
Member Author

justincy commented Mar 16, 2017

@justincy
Copy link
Member Author

I need to create a minimal reproduceable test case.

@justincy
Copy link
Member Author

The problem can be fixed (circumvented) by wrapping the API request in fs-person-portrait in a setTimeout to defer until the next tick. But I still don't understand why that's necessary when being use in fs-family and not anywhere else. In fs-family the fs-client elements are not ready until after their parents are and I can't replicate that behavior anywhere else.

I tried commenting out paper-card and leaving it's contents but the issue was still manifest which suggests it's not due to the light-dom timing changes in Polymer 2.

I see the right order of events inside fs-family if I comment out both paper-card and it's contents and instead add a single fs-person-chip.

<fs-person-chip person-id="KWWC-RCL"></fs-person-chip>

That suggests it's something with the complex configuration of fs-family.

I tried changing it from a hard-coded person-id to a binding on the person object, as is done normally in fs-family and the issue returned.

<fs-person-chip person="[[family.father]]"></fs-person-chip>

That narrows it down significantly. Something with the timing of property observers and bindings. I still don't understand how that affects the timing of fs-ready callbacks (including those that aren't immediate children) but I'm getting a lot closer.

@justincy
Copy link
Member Author

justincy commented Mar 20, 2017

🎉 I finally created a reproducable test case in JS Bin: http://jsbin.com/xexuturave/edit?html,console,output

@justincy
Copy link
Member Author

Reported the issue to Polymer at Polymer/polymer#4447

@justincy
Copy link
Member Author

The bug was confirmed by Polymer. There's not much to do until that's fixed.

@justincy
Copy link
Member Author

Polymer released a fix for the bug. I confirmed that it works. We're back in business.

@justincy
Copy link
Member Author

I hit an issue when upgrading the sample app. Polymer 2 doesn't guarantee any order of events for children and that's causing the client to not be configured properly. In v1 we relied on the order being guaranteed so we put the fs-client with configuration at the beginning and the client was correctly setup. But now that order isn't guaranteed, it's not necessarily being setup first so a non-configured client is being setup first and it creates the SDK instance with no config properties. Then when the configured fs-client is setup it's properties are ignored because the SDK instance is already setup. So either we need to find a way and rely on ordering or update the SDK to allow config properties to be changed after instantiation.

@justincy
Copy link
Member Author

Now that FamilySearch/fs-js-lite#28 added the config() method to the SDK we can stop relying on timing and just setup the SDK instance as we go.

@justincy
Copy link
Member Author

I updated fs-client so that it works regardless of order.

Next I need to address this issue as reported in the console:

paper-toolbar is deprecated. Please use app-layout instead!

@justincy
Copy link
Member Author

justincy commented May 3, 2017

There's one last issue. The person history dropdown is not updating properly. The dropdown lists updates as we browse but the label that shows the currently selected value when it's collapsed doesn't update properly.

After hours of investigation I believe it's caused by the DOM elements being recycled when the list changes. This causes observers to not detect changes (because the selected DOM element didn't change, just the contents did) and thus change events are not emitted and the dropdown menu doesn't get notified that the list changed.

@justincy
Copy link
Member Author

justincy commented May 3, 2017

Related: Polymer/polymer#4363

@justincy
Copy link
Member Author

justincy commented May 4, 2017

Bug report: PolymerElements/iron-selector#154

@justincy
Copy link
Member Author

justincy commented May 4, 2017

After figuring out exactly what the bug was, it was pretty easy to find a work around. So now we're all done with the v2 upgrade. 🎉

@justincy justincy closed this as completed May 4, 2017
@justincy justincy mentioned this issue Sep 1, 2017
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant