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

Closed
wants to merge 5 commits into
base: master
from

Conversation

@CaerusKaru
Member

CaerusKaru commented Aug 18, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot googlebot added the cla: yes label Aug 18, 2018

@vikerman vikerman requested review from vikerman and IgorMinar Aug 22, 2018

@IgorMinar

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>';

This comment has been minimized.

@IgorMinar

IgorMinar Aug 22, 2018

Member

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.

This comment has been minimized.

@gkalpak

gkalpak Aug 22, 2018

Member

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?

This comment has been minimized.

@CaerusKaru

CaerusKaru Aug 22, 2018

Member

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.

This comment has been minimized.

@IgorMinar

IgorMinar Aug 23, 2018

Member

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.

This comment has been minimized.

@CaerusKaru

CaerusKaru Aug 24, 2018

Member

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);');

This comment has been minimized.

@skreborn

skreborn Aug 22, 2018

Contributor

Why did you add extra spaces here? This seems like an accident.

This comment has been minimized.

@CaerusKaru

CaerusKaru Aug 22, 2018

Member

It’s not an accident, Domino changed how it serializes that particular form of CSS

This comment has been minimized.

@skreborn

skreborn Aug 23, 2018

Contributor

Well, that's an awfully strange change to make, but who am I to question their decision? All good then, thanks for the clarification!

@IgorMinar

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.

@IgorMinar

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.

@kara kara closed this in 3fb0da2 Sep 19, 2018

@CaerusKaru CaerusKaru deleted the CaerusKaru:domino21 branch Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment