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

[chore] replaces hard-coded minify values with vite.build.minify #4803

Merged

Conversation

Enteleform
Copy link
Contributor

@Enteleform Enteleform commented Sep 19, 2022

Changes

  • replaces hard-coded minify values with vite.build.minify, so that users can override minification from astro.config.

@changeset-bot
Copy link

changeset-bot bot commented Sep 19, 2022

🦋 Changeset detected

Latest commit: 300a0cb

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

This PR includes changesets to release 16 packages
Name Type
astro Patch
@e2e/astro-component Patch
@e2e/error-cyclic Patch
@e2e/error-react-spectrum Patch
@e2e/error-sass Patch
@e2e/errors Patch
@e2e/hydration-race Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/solid-recurse Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution Patch
@e2e/third-party-astro Patch

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 Sep 19, 2022
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.

Thanks!

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.

Explained in comment.

@matthewp
Copy link
Contributor

For server JS we are currently leaving that unminified main for easier debugging and because minification is not as necessary for server code. We might change that in the future but I don't think this PR is the right place for that.

So Instead I think using the option in the clientBuild is correct, but you might also need to check the option within the build-css vite plugin. So that it's used for CSS that's built as part of the server JS code.

@Enteleform
Copy link
Contributor Author

Enteleform commented Sep 19, 2022

So Instead I think using the option in the clientBuild is correct

Oh whoops, that's actually what I meant to do & mentioned in the initial comments @ Discord 😅. Just fixed it.

 

check the option within the build-css vite plugin. So that it's used for CSS that's built as part of the server JS code.

Should we just handle this for the client in this PR, and leave the server for another PR if it comes up?

Since you mentioned updating this @ ssrBuild would be more complex, I'm wondering if:

{clientJS:config, clientCSS:config, serverJS:static, serverCSS:static}

might make more sense in the meantime than

{clientJS:config, clientCSS:config, serverJS:static, serverCSS:config}

@matthewp
Copy link
Contributor

@Enteleform no, for CSS we can honor the minify option for both client and server. Since that's going to the client it should not be minified if you set it to false. The server JS is the only thing we want to always remain unminified.

@Enteleform
Copy link
Contributor Author

check the option within the build-css vite plugin

Just looked into it and I'm not completely sure what you're asking.

So there's astro:rollup-plugin-build-css-minify, which I've already updated. Then in same file there's astro:rollup-plugin-build-css, but it seems like there's presently no minification being applied.

Those are the only two references I've found to build-css in the codebase.

Are you saying that minification should be added @ rollup-plugin-build-css?

 

 

I did find another point of minification @ integrations/cloudflare.

Should I update that as well?

@matthewp
Copy link
Contributor

Cloudflare is minifying the server JS so no need to update there. This looks good now! Maybe I just misunderstood before.

packages/astro/src/core/build/static-build.ts Outdated Show resolved Hide resolved
packages/astro/src/core/build/vite-plugin-css.ts Outdated Show resolved Hide resolved
@matthewp matthewp merged commit f53d97d into withastro:main Sep 20, 2022
@astrobot-houston astrobot-houston mentioned this pull request Sep 20, 2022
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants