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
feat(platform-server): update domino to v2.1.0 #25564
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than the class=""
this looks good to me.
|
||
// This is a fix for a change in how Domino renders this on the server in v2.1.0 | ||
const source = toHtml(comp); | ||
const matches = source === '<div></div>' || source === '<div class=""></div>'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is weird. why would it serialize an empty string attribute? are we doing something that triggers this behavior?
my concern is that we'll be bloating the html size if we happen to do this on every element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about domino, but some attributes affect the element behavior even if empty.
The ||
is weird though. Are the tests run against multiple versions of Domino?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are run against Domino and against the browser. The browser does not exhibit this behavior, but Domino does, so the test needs to accept both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should understand why domino does this. Emitting these extra classes will cause HTML bloat which is undesirable.
Does this happen only on some elements or all elements? is this a bug in domino that we should report and fix?
I don't feel comfortable forcing people to update to domino 2.1 if it causes these kinds of issues, so before we do so, we need to understand the severity of this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class issue is only on this test because it does the following: create a div, attach a conditional class based on a boolean for the component (initialized to false), then set the boolean to true. In every other test, there is no bloat, but the act of initializing an empty class in Domino results in an empty string on the div.
@@ -588,7 +588,7 @@ class HiddenModule { | |||
expect(output).toContain('Works!'); | |||
expect(output).toContain('ng-trigger-myAnimation'); | |||
expect(output).toContain('opacity:1;'); | |||
expect(output).toContain('transform:translate3d(0, 0, 0);'); | |||
expect(output).toContain('transform:translate3d(0 , 0 , 0);'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add extra spaces here? This seems like an accident.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not an accident, Domino changed how it serializes that particular form of CSS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's an awfully strange change to make, but who am I to question their decision? All good then, thanks for the clarification!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed this with Vikram and the change looks good to me. The empty attribute issue doesn't seem to be widespread enough to warrant blocking this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed this with Vikram and the change looks good to me. The empty attribute issue doesn't seem to be widespread enough to warrant blocking this.
This PR however needs to be rebased before we can merge it.
4b63df1
to
5bec558
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information