search

ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

I2I: prettier for HTML #27198

Open alanorozco opened 4 years ago

alanorozco commented 4 years ago

Motivation

prettier is used for automatically formatting most code in this repo, except for HTML files.

When applying prettier directly, the AMP boilerplate will explode into many lines, even though it's usually written as one. This creates noise.

This issue describes a strategy for auto-formatting HTML files in this repository, while keeping the boilerplate written as one line.

State of <!-- prettier-ignore -->

<!-- prettier-ignore --> will only ignore the first node on the next line. This is problematic for AMP documents that hold a second <noscript> node in their boilerplate, which will not be ignored. This applies for every AMP format except ads.

A feature request for prettier introduces ignore range blocks for HTML. This would allow us to include any type of AMP boilerplate in one ignore section:

<head>
  <!-- prettier-ignore-start -->
  <style amp-boilerplate>/* ... */</style><noscript><style amp-boilerplate>/* ... */</style></noscript>
  <!-- prettier-ignore-end -->
</head>

Opportunities

Including the boilerplate in a prettier-ignore block would allow the Playground to use prettier (/cc @ampproject/wg-outreach)

/cc @ampproject/wg-infra

rsimha commented 4 years ago

From an infra point of view, I have no objection to using prettier for HTML files. I'll defer to @ampproject/wg-outreach for approval, since this will have to play well with ampproject/amp.dev code and tooling.

/cc @pbakaus @sebastianbenz @CrystalOnScript @mrjoro

sebastianbenz commented 4 years ago

From an amp.dev tooling perspective there's not problem using this approach. However, I'd prefer to avoid adding additional markup to example files as this just introduces more noise. It'd be fantastic if we could have an amp plugin for prettier (not sure if plugins are a thing).

Sidenote: the playground already supports formatting (via the {} button). It's implemented using js-beautifier and works by ignoring tags in the head.

samouri commented 4 years ago

I had actually tried using <!-- prettier-ignore-start --> just a couple days back and was surprised when it didn't work. I'd love for this to go through!

rsimha commented 4 years ago

I think there's a way to have our cake and eat it too with Prettier's custom parsers. I'd imagine we can override the behavior of the default html parser to not mess with AMP's boilerplate.

alanorozco commented 4 years ago

@rsimha Possibly a plugin too.

Are we okay with implementing custom behavior?

rsimha commented 4 years ago

If the custom behavior is amphtml specific and doesn't otherwise conflict with the default html parser, using a custom plugin SGTM.

This pattern is already followed by the dozens of custom eslint rules in our repo. None of them conflict with the standard eslint rules we use.

dreamofabear commented 4 years ago

Oh I thought we already did this. It already happens on save for me in VSCode.

rsimha commented 3 years ago

@alanorozco Is this still part of your plans?

samouri commented 3 years ago

Looks like the PR to add support for this in prettier has stalled based on a couple of bugs in the implementation. I'd personally vote against a custom solution in amphtml vs. assisting with upstreaming support.

alanorozco commented 3 years ago

An alternative put forth by @caroqliu: wrap the entire <head>

<!DOCTYPE html>
<html ⚡>
  <!-- prettier-ignore -->
  <head>
    <meta charset="utf-8" />
...

We lose formatting inside the <head> (style tags probably being the biggest loser) but maybe that's fine.

alanorozco commented 3 years ago

(Transfrom to apply once)

krdwan commented 3 years ago 
<!-- prettier-ignore -->

Used this in a few validator PRs which were being blocked without this. Because, autosave was expanding out the tags, it was causing the comparison files to become too large and was causing some failures in circle CI. Here are the PRs for reference:

https://github.com/ampproject/amphtml/pull/33478 https://github.com/ampproject/amphtml/pull/33480

alanorozco commented 3 years ago

@krdwan HTML format is not supposed to be enforced, do you have a link to the CI failures?

EDIT: Also not sure why your VS Code instance is auto-fixing .html files, it doesn't do it for me.

krdwan commented 3 years ago

@krdwan HTML format is not supposed to be enforced, do you have a link to the CI failures?

EDIT: Also not sure why your VS Code instance is auto-fixing .html files, it doesn't do it for me.

krdwan commented 3 years ago

@alanorozco Here is a link: https://app.circleci.com/pipelines/github/ampproject/amphtml/6685/workflows/e5ffd7eb-17cb-41ba-ad30-0c6ab1c74f7a/jobs/96158

I suppose my VSCode settings are slightly different.