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.

Sign up for GitHub

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

Jump to bottom

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

Closed
leonahliang90 opened this issue Dec 19, 2017 · 20 comments
Closed

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

leonahliang90 opened this issue Dec 19, 2017 · 20 comments
Assignees
@dreamofabear
Labels
P1: High Priority Type: Discussion/Question
Milestone
Sprint H2 February

Comments

@leonahliang90
Copy link
Contributor

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
@dreamofabear dreamofabear self-assigned this Dec 19, 2017
@dreamofabear
Copy link

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?

@dreamofabear dreamofabear changed the title amp-bind: Maximum number of bindings reached (2000). Additional elements with bindings will be ignored amp-bind: Maximum number of bindings reached Dec 19, 2017
@dreamofabear dreamofabear added this to the Backlog Bugs milestone Dec 19, 2017
@leonahliang90
Copy link
Contributor Author

leonahliang90 commented Dec 20, 2017 •
edited
Loading

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 ?

@dreamofabear
Copy link

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.

@dreamofabear
Copy link

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

@leonahliang90
Copy link
Contributor Author

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.

@leonahliang90
Copy link
Contributor Author

leonahliang90 commented Dec 31, 2017 •
edited
Loading

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>

@dreamofabear
Copy link

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.

@leonahliang90
Copy link
Contributor Author

leonahliang90 commented Jan 4, 2018 •
edited
Loading

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

@micyeung
Copy link

micyeung commented Jan 5, 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.

@dreamofabear
Copy link

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

@dreamofabear dreamofabear modified the milestones: Backlog Bugs, H1 January Jan 5, 2018
@dreamofabear dreamofabear changed the title amp-bind: Maximum number of bindings reached amp-list: [src] changes don't properly dispose old bindings Jan 6, 2018
@dreamofabear
Copy link

Stumbled on a couple more bugs. 😄

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

@dreamofabear dreamofabear mentioned this issue Jan 6, 2018
Merged
@dreamofabear
Copy link

@leonalicious Verified this is fixed on version 1515455265699.

@micyeung
Copy link

@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

@dreamofabear
Copy link

dreamofabear commented Jan 12, 2018 •
edited
Loading

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".

@leonahliang90
Copy link
Contributor Author

I am using Powered by AMP ⚡ HTML – Version 1515614886756

@micyeung
Copy link

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
Copy link

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

@dreamofabear
Copy link

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

Tunnel 0f4218e2.ngrok.io not found

@dreamofabear dreamofabear reopened this Jan 18, 2018
@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. @choumx Do you have any updates?

@dreamofabear
Copy link

Planning to tackle this in the sprint after AMP Conf.

@dreamofabear dreamofabear mentioned this issue Feb 28, 2018
Merged
@leonahliang90 leonahliang90 mentioned this issue May 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dreamofabear dreamofabear

Labels
P1: High Priority Type: Discussion/Question
Projects
None yet
Milestone
Sprint H2 February
Development

No branches or pull requests
4 participants
@leonahliang90 @micyeung @dreamofabear @ampprojectbot