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

amp-script: Improve messaging for non-mutating AMP.setState #26816

Closed
morsssss opened this issue Feb 14, 2020 · 5 comments
Closed

amp-script: Improve messaging for non-mutating AMP.setState #26816

morsssss opened this issue Feb 14, 2020 · 5 comments

Comments

@morsssss
Copy link
Contributor

As per discussion in #26401 and #26524 :

Currently, in an container that's variable-sized (as defined in #26524), AMP.setState() is simply not allowed on load, even when that state change doesn't cause a DOM mutation. In other words, it's not allowed even when the state change wouldn't change anything the user could see.

Is it worth allowing this, perhaps by simply not following through on binds in such cases? Or is that even more confusing?

@samouri
Copy link
Member

samouri commented Feb 25, 2020

Is it worth allowing this, perhaps by simply not following through on binds in such cases? Or is that even more confusing?

I'm pretty sure thats exactly what we are currently doing. We change state, but don't trigger bind reevaluation. In this case, you'll see a warning like this:

[amp-script] AMP.setState only updated page state and did not reevaluate bindings due to lack of recent user interaction.

@morsssss
Copy link
Contributor Author

I see! I think this was changed after we discovered this issue, but before I reported it 😈

I'd like to clarify this error message, though. Maybe something like this:

AMP.setState updated the state variable. But criteria for recent user interaction were not met, so bindings weren't updated.

Or perhaps:

Since user interaction and fixed-size container criteria weren't met, page bindings weren't updated after setState().

We need a better - and standardized - phrase for "criteria for recent user interaction", which could also be used in places like this.

@kristoferbaxter
Copy link
Contributor

The longer these error messages get, the more we need to move them to documents and urls in the console.

Jake, do you want to have a chat about taking over the work that was started here?

@morsssss
Copy link
Contributor Author

I had that concern too. I don't want to waste bytes!

A link or standard phrase could replace "Since user interaction and fixed-size container criteria weren't met". I've also written up an explanation of when mutations are and are not allowed, which I've got in a Doc right now. That could be published, and these could refer to that.

@dreamofabear dreamofabear changed the title Let <amp-script> make state changes that don't result in DOM mutation amp-script: Improve messaging for non-mutating AMP.setState Mar 3, 2020
@stale
Copy link

stale bot commented Aug 29, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Aug 29, 2021
@stale stale bot closed this as completed Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants