🐛 compiler.js buildDoms: ensure behavior matches browser #37671
Conversation
samouri
changed the title
samouri
force-pushed
the
test-compiler
branch
from
2da64fd
to
8259ba7
Compare
|
Hey @Jiaming-X, @jeffkaufman! These files were changed: Hey @jridgewell! These files were changed: |
| img.style.width = '120px'; | ||
| img.style.height = '100px'; | ||
| img.id = 'img-' + i; | ||
| img.style.setProperty('width', '120px'); |
There was a problem hiding this comment.
In actual component code consumed by buildDom, are property assignments like el.style.width = '120px'; allowed or will it break if they don't use the setter methods?
There was a problem hiding this comment.
Do we have lint rules in place yet to enforce this, or any docs/guidance in the main repo?
There was a problem hiding this comment.
Nope! I discovered it this AM, so nothing yet.
The unit tests added in this PR will ensure we don't regress for the 3 components covered, but will sync up at standup to see if there is anything we should do in addition.
Ideally we can fix the root cause in worker-dom.
Even if its a solution we only apply server-side.
| * @return {Element} | ||
| */ | ||
| function getAmpSlideScroll( | ||
| opt_hasLooping, | ||
| opt_slideCount = 5, | ||
| opt_attachToDom = true, | ||
| opt_hasAutoplay = false, | ||
| opt_autoplayLoops | ||
| opt_autoplayLoops, | ||
| doc = env.win.document |
There was a problem hiding this comment.
Can you have an arg with a default after an arg without one?
Also, you're gonna hate this, but there are now a total of 6 optional arguments...meaning...options object time perhaps?
There was a problem hiding this comment.
I don't hate it! I actually completely agree with it!
I'd insist on this though that I'd prefer to do it in a separate PR.
There was a problem hiding this comment.
forgot to answer the earlier q, yes you can. all a default arg means is: “if provided undefined, default to X value”
| /* hasLooping */ true, | ||
| /* slideCount */ undefined, | ||
| /* attachToDom */ false, | ||
| /* opt_hasAutoPlay */ undefined, | ||
| /* opt_autoplayLoops */ undefined, |
There was a problem hiding this comment.
Happy to commit to doing this in a foLLowUp Pr.
Worried if I do it here it will distract from the primary goals
| const browserHtml = getDeterministicOuterHTML(browserDiv); | ||
| const workerHtml = getDeterministicOuterHTML(workerDomDiv); | ||
| expect(workerHtml).equal(browserHtml); |
There was a problem hiding this comment.
This is a super common pattern with some typo potential. Might be more useful to directly export expectDeterministicOuterHtmlEqual(worker, browser)
There was a problem hiding this comment.
Considered this! Don't love the pattern of making helpers that perform assertions. I know when I read it in other folk's code it always takes me longer to understand the test case.
I have a preference for under-abtracting in unit tests (keeping them DAMP). That said, especially in this case, don't have a very strong opinion. Will test it out locally and see how it seems.
There was a problem hiding this comment.
I likewise don't feel too strongly, esp given how few components will likely do ths
samouri
force-pushed
the
test-compiler
branch
from
7eb70ae
to
35b9727
Compare
|
|
||
| /** | ||
| * Returns the outerHTML for an element, but with lexicographically sorted attributes. | ||
| * @param {Node} node |
There was a problem hiding this comment.
I think this has broken type check on main. Should probably be HTMLElement. check-types is not configured to run for this file, but it probably should be.
* compiler.js buildDoms: ensure behavior matches browser * document the hack * fix test, also update compiler-hydration with latest. * fix more broken unit tests * pr feedback updates (cherry picked from commit b875822)
summary
While testing server-side transforms, I noticed a bug in
worker-dom. Specifically, setting keys of thestyleprop on a node does not reflect in attributes properly. Instead style.setProperty has to be used to trigger the "PropertyBackedAttribute" flow.Separately, if we actually need JS in
setStylesto dynamically figure out vendor-specific prop names, that of course won't work either.This PR does a few things:
width,height,order) to be set withsetProperty. I scanned the relevant components we plan on server-rendering and I believe this covers it.getDeterministicOuterHTMLthat behaves similarly toel.outerHTMLexcept sorts the attributes. Note that this doesn't deterministically print tokens withinstyle=attr, but that hasn't been an issue yet and can be added later.amp-carousel,amp-fit-text, andamp-layoutto ensure WorkerDOM behavior matches browser. Also adds a similar test to the builder wrapper, which for now mostly means checkingapplyStaticLayout.compiler-hydration.htmlwith the results of actually running the "client render" HTML throughcompiler.js.