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

feat(core): allow opting out of HTML minification #7581

Merged
merged 1 commit into from Jun 15, 2022

Conversation

alexandernst
Copy link
Contributor

@alexandernst alexandernst commented Jun 7, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

The current build process is "optimizing" the HTML in a way that breaks bootstrap. More specifically, it removes some "worthless" attributes (such as type="text" from input fields), which breaks Bootstrap's [type="text"] selectors (probably selectors of other css frameworks aswell).

This change will allow users to specify the exact options that they want to be passed to the minifier. It won't introduce any breaking changes since it's merging the existing opts with any user-provided opts.

Test Plan

N/A

Test links

Related issues/PRs

N/A

@netlify
Copy link

netlify bot commented Jun 7, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 4347c7f
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62a0e61f42506500084a7bda
😎 Deploy Preview https://deploy-preview-7581--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Jun 7, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 69 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 81 🟢 100 🟢 100 🟢 100 🟢 90 Report

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

When we were discussing over Discord, I'm not really sure whether we want a simple env var that toggles minification on/off, or a full config option. I'm strongly leaning towards the former because it isn't that well-used at all and making that many options for it seems like an overkill. Remember that we try to minimize the API surface as much as possible.

That's my strong opinion, not sure about @slorber's, but I think we definitely need to use env vars instead, maybe even 6 env vars, but never a config option.

examples/classic/docusaurus.config.js Outdated Show resolved Hide resolved
@alexandernst
Copy link
Contributor Author

a simple env var that toggles minification on/off

I get that you're trying to keep the API surface as simple as possible, but a single "on" or "off" switch that enables or disables the optimization / minification of everything (html, css and js) is just not useful. There is a bug (that I described in the PR) that happens when a particular optimization is performed. That doesn't mean that I want absolutely every optimization disabled.

Plus, the API surface is still simple, the entire buildConfig hashmap is optional; and at the same time it also providers flexibility to the users who might want to tweak the optimization step.

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Jun 8, 2022

but a single "on" or "off" switch that enables or disables the optimization / minification of everything (html, css and js) is just not useful

No, only a single env var that toggles HTML minification. CSS and JS minification is controlled by another argument (--no-minify) already. Also, I'm okay with 6 env vars, just not a config option—env vars are implicit API and can get as big as we want. This is what we do with other build options: we have a bunch of env var APIs that control these optimizations already (e.g. we have DOCUSAURUS_SSR_CONCURRENCY, WEBPACK_URL_LOADER_LIMIT, USE_SIMPLE_CSS_MINIFIER).

@alexandernst
Copy link
Contributor Author

Oh, I see. Okey! Should I use a single env var in order to skip the entire minify() call? Or do you want me to use an env var for each of the settings that are currently passed? (check code snippet below)

        {
          removeComments: false,
          removeRedundantAttributes: true,
          removeEmptyAttributes: true,
          removeScriptTypeAttributes: true,
          removeStyleLinkTypeAttributes: true,
          useShortDoctype: true,
          minifyJS: true,
        },

@Josh-Cena
Copy link
Collaborator

I'm fine with either, but not sure about @slorber's preference. Let's use a single env var first to skip the entire minify call, shall we?

@Josh-Cena Josh-Cena changed the title feat(core): Implement BuildConfig opt feat(core): allow opting out of HTML minification Jun 8, 2022
@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Jun 8, 2022
@alexandernst
Copy link
Contributor Author

@Josh-Cena I just force-pushed the new changes, 🙏👀

@slorber
Copy link
Collaborator

slorber commented Jun 8, 2022

I'm fine with env vars with very explicit names used as escape hatches here and there 👍

To me, those things do not even have to be documented at all, as most users don't need it anyway, and not documenting it is also an opportunity for us to collect feedback on how it is or will be used. If it becomes useful to a lot of users and we have enough feedback, we can move it to a first-class option.

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

LGTM—@slorber wants to remove the documentation, so I'll see if he absolutely want that change before merge.

@slorber
Copy link
Collaborator

slorber commented Jun 15, 2022

that should be fine thanks 👍

@slorber slorber merged commit b503523 into facebook:main Jun 15, 2022
@alexandernst alexandernst deleted the buildConfig branch June 15, 2022 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants