amp-script: Filter user-visible mutations during hydration #26401
Conversation
dreamofabear
force-pushed
the
hydrate-mutate-bug
branch
from
f06a5e1
to
240a03e
Compare
| errors[type] = errors[type] + 1 || 1; | ||
| }); | ||
| // Emit an error message for each mutation type, including count. | ||
| Object.keys(errors).forEach(type => { |
There was a problem hiding this comment.
nit: Object.entries would give you both key and value, for a more concise loop.
There was a problem hiding this comment.
Not safe to use unfortunately. https://caniuse.com/#feat=object-entries
Looks like we ban it in conformance-config.textproto too.
| // However, gesture-backed (e.g. click) mutations are OK. | ||
| const button = await controller.findElement('#mutate button'); | ||
| controller.click(button); | ||
|
|
||
| const h1 = await controller.findElement('#mutate h1'); | ||
| await expect(controller.getElementText(h1)).to.contain('Lorem Ipsum'); |
There was a problem hiding this comment.
only the first part of this test is actually for the 'hydration' phase. The 'click' occurs afterwards, and is really testing that the event listener was added. I think thats fine, since the second half is still useful (test that adding event listeners still works)
There was a problem hiding this comment.
Yea, as you mention the distinction for "hydration" is not really useful. It's only mentioned here due to the bug.
| // In layout=container, amp-script requires mutations to be backed by | ||
| // user gestures. This ensures that this requirement is also enforced | ||
| // on load AKA "hydration". | ||
| it('should not mutate on load in layout=container', async () => { |
There was a problem hiding this comment.
Is there anywhere where we are testing the inverse? e.g. it('should mutate on load in layout=fixed').
There was a problem hiding this comment.
Yea, the first two tests cover that case.
|
question! why do we special case hydration? |
Right, this PR kind of removes it. The remainder is probably what you're suggesting: https://github.com/ampproject/amphtml/pull/26401/files#diff-2309faf5b32ca308fd2bb6e38e44d29eR440 |
|
Do Preact/React need to be able to take advantage of the hydration phase mutations to create their dom elements? e.g. if someone were using <amp-script id="root" height="400" width="400" script="preact-script"></amp-script>
<script type="text/plain" target="amp-script">
import { render } from 'preact';
function MyApp() {
...
}
const root = document.getElementById('root');
render(<MyApp/> root)
</script> |
|
Correct, you need to configure Preact/React to use SSR mode. https://reactjs.org/docs/react-dom.html#hydrate |
ampprojectbot
added
PR use: In Beta / Experimental
and removed
PR use: In Beta / Experimental
labels
|
And this will apply to all |
|
This only applies to the dynamic size amp-scripts ( Fixed size components still have no restrictions |
|
Thanks! /cc @AndrewKGuan |
Fixes #26375. Requires ampproject/worker-dom#764.