Amphtml: amp-list: [src] changes don't properly dispose old bindings

Created on 19 Dec 2017 · 20Comments · Source: ampproject/amphtml

I am currently facing a Maximum number of bindings reached (2000). Additional elements with bindings will be ignored issue.

based on my research, at [bind-impl.js#L532]
(https://github.com/ampproject/amphtml/blob/master/extensions/amp-bind/0.1/bind-impl.js#L532) will change the limitExceeded from false to true,
and this causes me an error which is = 106 + 1 > 106 when i am trying to select countries on my 4th or 5th times.
ss1
ss2
ss3

can you please elaborate more on the L530, what is this IF statement checking bindings.length + boundProperties.length > limit does ? I am also curious how does this IF check is relevant to 2000 bindings numbers.

Appreciate more sharing from you. Thanks in advanced.
@choumx

High Priority DiscussioQuestion

Source

leonalicious

All 20 comments

L530 checks that the bindings found so far (bindings) + the new bindings in the scanned node (boundProperties) exceed the total limit of 2000.

Abstractly this means that on any AMP page, you can only have 2000 bindings e.g. [property]. Can you share the AMP page that's hitting this limit?

choumx on 19 Dec 2017

I have changed my code design and I don't get this error anymore.
I was using amp-list and the [src] was set to an array, which was generated by usingamp-form. So by using the previous approach, everytime when I AMP.setState any state#id, it will trigger ENTIRE state and affect my amp-list [src], and force to re-render the entire amp-list HTML DOM. Besides this issue, I also notice there are several cases when I AMP.setState some state, this action will erase default value of my other state and causes them to null. Are these normal ?

leonalicious on 20 Dec 2017

Glad to hear that you found a solution!

everytime when I AMP.setState any state#id, it will trigger ENTIRE state and affect my amp-list [src], and force to re-render the entire amp-list HTML DOM.

Interesting. This shouldn't happen if the array itself didn't change (with some limits).

Besides this issue, I also notice there are several cases when I AMP.setState some state, this action will erase default value of my other state and causes them to null.

It shouldn't erase values of other variables since setState(foo) deep-merges foo into the current state. What could be happening is that some elements use variables that are uninitialized which evaluates to null.

choumx on 20 Dec 2017

@leonalicious Is this still an issue or can it be closed?

choumx on 28 Dec 2017

hey William, actually we are still facing maximum 2000 bindings issue, please give me some time and I will be writing a demo for this. you may also close this if you want.

leonalicious on 29 Dec 2017

Hi William, is it any <html> render come after the genesis <html> Tree render will be treated as new amp binding ?

By looking at the screenshot I attached in this issue, I suspect whenever <amp-list>'s [src] is changed, entire <amp-list> will be re-rendered, and it will increase the BINDING NUMBER upon each child render within the <amp-list>

leonalicious on 31 Dec 2017

That's correct, but the bindings in previously rendered elements will be removed. "Total number of bindings" should exactly equal the number of [prop] attributes on the page at any given moment.

choumx on 31 Dec 2017

Currently there are 2 <amp-list>, for our case, attribute <amp-list> is relying on the category <amp-list> and will be fetching new HTTP request upon users select on attribute

The first gif is 30 seconds which shows how to reproduce the error, basically, what i did was to click between two categories
ezgif com-video-to-gif

That's correct, but the bindings in previously rendered elements will be removed. "Total number of bindings" should exactly equal the number of [prop] attributes on the page at any given moment.

This is weird, because based on our understanding, the previous bindings should be removed, that's why I am only clicking between two categories

And the gif below shows how the error appears.
ezgif com-video-to-gif 1

Any ideas ? @choumx

leonalicious on 4 Jan 2018

A demo page has been set up at https://randomtestapp-a5a6f.firebaseapp.com/amp-binding-limit-exceeds-issue.html.
It contains a master amp-list and a detail amp-list, and clicking between items in the master amp-list will trigger a "Max number of bindings reached" error.

micyeung on 5 Jan 2018

👍1

Thanks for sharing that example. This is definitely a bug and can be fixed.

choumx on 6 Jan 2018

👍1

Stumbled on a couple more bugs. 😄

Duplicate rescan due to AmpEvents.DOM_UPDATE event.
Duplicate amp-list render/fetch due to race between mutatedAttributesCallback and layoutCallback.

choumx on 6 Jan 2018

👍1

@leonalicious Verified this is fixed on version 1515455265699.

choumx on 9 Jan 2018

👍1

@choumx
There's another issue found with 2000 max bindings; issue described in further detail with sample code here:
https://github.com/aemsite/ampdemo/tree/master/amp-binding-limt-exceeds-with-form

micyeung on 12 Jan 2018

I can't repro the bug with that example.

@leonalicious What version of AMP are you running? I can't find the RTV in the GIF ending with "6756".

choumx on 12 Jan 2018

I am using Powered by AMP ⚡ HTML – Version 1515614886756

leonalicious on 13 Jan 2018

Fired up a demo at https://0f4218e2.ngrok.io/index.html
Clicking 7-8 times between the first 2 rows in the master list will trigger Max Bindings failure.

micyeung on 13 Jan 2018

@choumx would you have an update on the bug here, based on the demo at https://0f4218e2.ngrok.io/index.html ?

micyeung on 17 Jan 2018

Looks like there's still a lingering race condition trigger for this bug. Also your tunnel is down: