Skip to content
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

HTML minification #6706

Merged
merged 40 commits into from
May 17, 2023
Merged

HTML minification #6706

merged 40 commits into from
May 17, 2023

Conversation

JerryWu1234
Copy link
Contributor

@JerryWu1234 JerryWu1234 commented Mar 30, 2023

Changes

  • Implementation for HTML Minification roadmap#537
  • Adds the compressHTML flag to the config, defaulting to false.
  • This value is passed to the compiler's compact flag.

Testing

  • Test case added for dev and build which verifies the HTML is compressed by checking the number of lines in the output (should be 1).

Docs

  • Config value is documented in the types.

@changeset-bot
Copy link

changeset-bot bot commented Mar 30, 2023

🦋 Changeset detected

Latest commit: 756b68e

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Mar 30, 2023
@JerryWu1234
Copy link
Contributor Author

@matthewp cc

@JerryWu1234 JerryWu1234 marked this pull request as ready for review April 26, 2023 13:04
@JerryWu1234 JerryWu1234 requested a review from a team as a code owner April 26, 2023 13:04
@matthewp matthewp changed the title TDD pattern development HTML minification Apr 28, 2023
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, @wulinsheng123! One little paragraph raised a couple of questions below. 😅

I'm going to take this opportunity to converge on some standardizing of how we define these Booleans, so thanks for being my guinea pig and putting a little extra work in here! I've offered a couple of suggestions for wording that match the structure of other terms on this page, and want to double check what the default value actually is (true or false) in order to make the example as helpful and realistic as possible.

I leave it in y'all's hands to come up with something reasonable based on my feedback! 🙌

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Show resolved Hide resolved
@matthewp
Copy link
Contributor

@wulinsheng123 can you add a test for dev? I know you said it wasn't working there. Then @bluwy or myself will test out what the problem is.

@JerryWu1234
Copy link
Contributor Author

JerryWu1234 commented May 4, 2023

@wulinsheng123 can you add a test for dev? I know you said it wasn't working there. Then @bluwy or myself will test out what the problem is.

Please tell me a clue on how to test for dev. because I have tried a lot, there is no better idea. But I'm definitely sure I have passed that property to the compiler.

@matthewp
Copy link
Contributor

matthewp commented May 4, 2023

@wulinsheng123 The fixture object has a way to create a dev server. Then you can make requests against it, similar to how you do in the build. Here's an example test file that tests both build and dev:

devServer = await fixture.startDevServer();

@matthewp
Copy link
Contributor

matthewp commented May 8, 2023

I submitted the stage 3 RFC here: withastro/roadmap#581

@matthewp matthewp added the semver: minor Change triggers a `minor` release label May 8, 2023
packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
packages/astro/test/astro-minification-html.test.js Outdated Show resolved Hide resolved
packages/astro/test/astro-minification-html.test.js Outdated Show resolved Hide resolved
packages/astro/test/astro-minification-html.test.js Outdated Show resolved Hide resolved
packages/astro/test/astro-minification-html.test.js Outdated Show resolved Hide resolved
packages/astro/test/astro-minification-html.test.js Outdated Show resolved Hide resolved
packages/astro/test/astro-minification-html.test.js Outdated Show resolved Hide resolved
packages/astro/test/astro-minification-html.test.js Outdated Show resolved Hide resolved
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should add a test for SSR too, what do you think?

matthewp and others added 8 commits May 9, 2023 15:42
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! 🚀

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks great! Have one small nit below.

packages/astro/test/astro-minification-html.test.js Outdated Show resolved Hide resolved
packages/astro/test/astro-minification-html.test.js Outdated Show resolved Hide resolved
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking to prevent accidental merge before release day.

matthewp and others added 2 commits May 15, 2023 13:30
@matthewp matthewp dismissed their stale review May 17, 2023 13:07

Merge day!

@matthewp matthewp merged commit 763ff2d into withastro:main May 17, 2023
@astrobot-houston astrobot-houston mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants