Add html to prettier #13252

swarajsaaj · 2020-12-12T15:14:41Z

HTML files will now be included in prettier command

Please make sure the below checklist is followed for Pull Requests.

All continuous integration tests are green
Tests are added where necessary
jhipster-online was updated if necessary
Documentation is added/updated where necessary
Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

mshima

LGTM.

mshima · 2020-12-12T17:51:05Z

@swarajsaaj move html extension to inside skipClient condition.
I don't think there is any html on backend only.

swarajsaaj · 2020-12-12T18:03:54Z

@swarajsaaj move html extension to inside skipClient condition.
I don't think there is any html on backend only.

@mshima I could find src/main/resources/templates/error.html in backend, also some mail templates are there for usermanagement apps in src/main/resources/templates/mail/

gmarziou · 2020-12-12T18:20:35Z

I could find src/main/resources/templates/error.html in backend, also some mail templates are there for usermanagement apps in src/main/resources/templates/mail/

But these are Thymeleaf templates, I'm not sure Prettier can do a good job with them although I did not check.
I have same doubts about Angular templates, usually code formatters are not comfortable with template syntax.

swarajsaaj · 2020-12-12T18:39:51Z

I could find src/main/resources/templates/error.html in backend, also some mail templates are there for usermanagement apps in src/main/resources/templates/mail/

But these are Thymeleaf templates, I'm not sure Prettier can do a good job with them although I did not check.
I have same doubts about Angular templates, usually code formatters are not comfortable with template syntax.

@gmarziou I think for Thymeleaf most of syntax is inserted as attributes to HTML elements (with a prefix namespace), so they should not interfere with formatting.

For Angular templates, Prettier seems to have support for *.component.html files since v1.15 https://prettier.io/blog/2018/11/07/1.15.0.html to take interpolation into consideration

mshima · 2020-12-12T18:43:14Z

@gmarziou you can see the result at https://github.com/jhipster/generator-jhipster/pull/13252/checks?check_run_id=1542973740 (MERGE: project diff section).

Backend files are the first. Looks ok to me.

gmarziou · 2020-12-12T22:27:11Z

Thanks for the examples, output is consistent and works well with templates.

It's just that I'm not a big fan of the way Prettier formats html which uses much more vertical space than original formatting.

            <input
              type="password"
              class="form-control"
              id="newPassword"
              name="newPassword"
              placeholder="{{ 'global.form.newpassword.placeholder' | translate }}"
              formControlName="newPassword"
              data-cy="resetPassword"
              #newPassword
            />

or

<html
  xmlns:th="http://www.thymeleaf.org"
  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://www.thymeleaf.org"
  th:lang="${#locale.language}"
  lang="en"
>

Also embedded CSS formatting does not look familiar to me, I would keep body, p { together rather than inserting a new line like below:

      @media only screen and (max-width: 280px) {
        body,
        p {

mshima · 2020-12-12T23:08:45Z

Not a big fan either, but overall is better with prettier enabled IMO.

gmarziou · 2020-12-13T09:55:04Z

Agreed, let's favor style consistency.

HTML files will now be included in prettier command Fix jhipster#13090

swarajsaaj · 2020-12-13T18:18:53Z

@mshima @gmarziou Could you please help why "npm test" is failing in the CI (https://github.com/jhipster/generator-jhipster/pull/13252/checks?check_run_id=1546315589) ? I am not sure how this timeout error can be due to html prettier change, or is it due to some other issue?

mshima · 2020-12-13T18:37:06Z

Try to increase timeout to 40000 at:

generator-jhipster/.mocharc.js

Line 5 in 7b07d28

timeout: 30000,

Enabling prettier at html can make the generation slower.

Increasing timeout for mocha tests due to failure in CI as HTML formatting may increase generation time. Fix jhipster#13090

Fix jhipster#13090

swarajsaaj · 2020-12-14T07:34:15Z

@mshima I have tried increasing 30000ms to 90000ms in steps, but seems that is not the issue (original test runs in ~3s), seems there is some conflict with HelloVue.html file being generated in the test (

generator-jhipster/test/blueprint/entity-client-blueprint.spec.js

Line 25 in 355fb16

    
           this.template(path.join(process.cwd(), 'HelloVue.html.ejs'), `${ANGULAR_DIR}HelloVue.html`);

)

Correct HelloVue.html syntax to <head> tag instead of nested <meta> tags, and reverted mocha tests timeout to 30000ms are original Fix jhipster#13090

swarajsaaj · 2020-12-14T19:32:52Z

@mshima Finally found the hidden problem, under the hood in test prettier was getting stuck due to a syntax error in HelloVue.html, <meta> was enclosed in a nested meta tag instead of a <head>, thanks for the help :)

DanielFran · 2020-12-15T10:10:49Z

@swarajsaaj Thanks for the contribution!

mshima · 2020-12-15T10:12:58Z

Great job finding the error.
I actually didn’t help.

DanielFran requested a review from mshima December 12, 2020 15:23

mshima approved these changes Dec 12, 2020

View reviewed changes

Add html to prettier

163f3cf

HTML files will now be included in prettier command Fix jhipster#13090

swarajsaaj force-pushed the add-html-prettier branch from d350332 to 163f3cf Compare December 13, 2020 17:45

swarajsaaj added 4 commits December 14, 2020 00:11

Increase mocha timeout to 40000ms

a1d3184

Increasing timeout for mocha tests due to failure in CI as HTML formatting may increase generation time. Fix jhipster#13090

Increase mocha timeout to 50000ms

6b4498d

Fix jhipster#13090

Increase mocha timeout to 60000s

36c88e0

Fix jhipster#13090

Increase mocha timeout to 90000ms

5d989e0

Fix jhipster#13090

Correct HelloVue.html syntax and revert mocha timeout

6b8d469

Correct HelloVue.html syntax to <head> tag instead of nested <meta> tags, and reverted mocha tests timeout to 30000ms are original Fix jhipster#13090

DanielFran merged commit 580b200 into jhipster:main Dec 15, 2020

pascalgrimaud added this to the 7.0.0-beta.0 milestone Dec 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add html to prettier #13252

Add html to prettier #13252

swarajsaaj commented Dec 12, 2020

mshima left a comment

mshima commented Dec 12, 2020

swarajsaaj commented Dec 12, 2020

gmarziou commented Dec 12, 2020 •

edited

swarajsaaj commented Dec 12, 2020 •

edited

mshima commented Dec 12, 2020

gmarziou commented Dec 12, 2020 •

edited

mshima commented Dec 12, 2020

gmarziou commented Dec 13, 2020

swarajsaaj commented Dec 13, 2020 •

edited

mshima commented Dec 13, 2020

swarajsaaj commented Dec 14, 2020 •

edited

swarajsaaj commented Dec 14, 2020

DanielFran commented Dec 15, 2020

mshima commented Dec 15, 2020

Add html to prettier #13252

Add html to prettier #13252

Conversation

swarajsaaj commented Dec 12, 2020

mshima left a comment

Choose a reason for hiding this comment

mshima commented Dec 12, 2020

swarajsaaj commented Dec 12, 2020

gmarziou commented Dec 12, 2020 • edited

swarajsaaj commented Dec 12, 2020 • edited

mshima commented Dec 12, 2020

gmarziou commented Dec 12, 2020 • edited

mshima commented Dec 12, 2020

gmarziou commented Dec 13, 2020

swarajsaaj commented Dec 13, 2020 • edited

mshima commented Dec 13, 2020

swarajsaaj commented Dec 14, 2020 • edited

swarajsaaj commented Dec 14, 2020

DanielFran commented Dec 15, 2020

mshima commented Dec 15, 2020

gmarziou commented Dec 12, 2020 •

edited

swarajsaaj commented Dec 12, 2020 •

edited

gmarziou commented Dec 12, 2020 •

edited

swarajsaaj commented Dec 13, 2020 •

edited

swarajsaaj commented Dec 14, 2020 •

edited