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(adapter-node): precompress (gzip & brotli) assets #1693

Merged
merged 3 commits into from
Jun 16, 2021

Conversation

sastan
Copy link
Contributor

@sastan sastan commented Jun 15, 2021

sirv supports using precompressed assets but they are not generated during the build. This PR allows to precompresses html, js, json, css, svg, and xml assets and prerenderd pages using gzip and brotli during the adapt phase which allows sirv to use these.

It introduces a new option precompress?: boolean to enable to precompression. It defaults to false - i.e. you must opt-in to generate precompressed assets.

This PR adds support for precompressed pre-rendered pages as well.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

@changeset-bot
Copy link

changeset-bot bot commented Jun 15, 2021

🦋 Changeset detected

Latest commit: 37dc6d9

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-node 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

@sastan sastan force-pushed the adapter-node-precompress-assets branch from c3b04cd to ea6372d Compare June 15, 2021 08:01
@dominikg
Copy link
Member

have you seen this PR that was recently merged? #1672 The discussion mentions how to add precompression with a custom script or other plugins.

If automatic pre-compression was added to sveltekit i think it should be configurable and default to disabled
as it adds significant overhead to the build.

@sastan
Copy link
Contributor Author

sastan commented Jun 15, 2021

Yes I saw that PR and it is the reason for this PR. I currently use an additional script after the build but I think it would be nice to have it included in the build because precompressed assets are supported during runtime (sirv) but not within the build step.

I agree that it should be configurable and will adjust the PR accordingly.

@sastan sastan force-pushed the adapter-node-precompress-assets branch from ea6372d to f873cfd Compare June 15, 2021 08:46
@dominikg
Copy link
Member

There are a lot more options/settings that could be considered when pre-compressing:

  • are there other adapters that would benefit from pre-compression
  • which files get compressed (filters, extensions, additional files?)
  • size limits (min, max filesize to skip compression, omit result if not smaller by X)
  • enable br, gz or both
  • advanced algorithm options
  • zopfli for gzip
  • parallelization of compression (max concurrent)
  • are there other compression methods that could be useful (eg svgo, png optimization)

This is of course too much to add to adapter node and a simple boolean with sensible defaults is ok imho. What those defaults are and if it should live somewhere else i'm not sure.
Maybe some documentation for how to use custom scripts / plugins would be sufficient too?

other compression plugins:
https://github.com/alloc/vite-plugin-compress
https://github.com/anncwb/vite-plugin-compression

@sastan
Copy link
Contributor Author

sastan commented Jun 15, 2021

I agree. They are a lot of possible options. I believe a sensible default like proposed in this PR would make sense as it covers the most common case – text file compression using gzip and brotli.

As it is opt-in users could implement their own advanced solution or enable the 80% solution provided by the plugin.

Do you think this PR has a chance of merging?

If that is the case should I add the following?

  • files smaller than 1501 bytes are not compressed, since the MTU of a TCP packet is 1500 bytes

@sastan sastan force-pushed the adapter-node-precompress-assets branch 2 times, most recently from 9b875f4 to f2d074c Compare June 15, 2021 17:25
Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

fwiw, i dont think we need to implement a p-map-like throttler

packages/adapter-node/index.js Outdated Show resolved Hide resolved
packages/adapter-node/index.js Show resolved Hide resolved
@sastan sastan force-pushed the adapter-node-precompress-assets branch from f2d074c to f4104d7 Compare June 15, 2021 19:33
@bjon
Copy link
Contributor

bjon commented Jun 15, 2021

Brotli and GZ don't seem to be enabled for prerendered_handler. Maybe you should fix that as well?

@sastan
Copy link
Contributor Author

sastan commented Jun 15, 2021

Brotli and GZ don't seem to be enabled for prerendered_handler. Maybe you should fix that as well?

I'll take a look.

@sastan sastan force-pushed the adapter-node-precompress-assets branch from f4104d7 to fb3ed4d Compare June 15, 2021 19:51
@sastan
Copy link
Contributor Author

sastan commented Jun 15, 2021

Brotli and GZ don't seem to be enabled for prerendered_handler. Maybe you should fix that as well?

Done.

@lukeed
Copy link
Member

lukeed commented Jun 15, 2021

Just an FYI – it's much harder to follow/track PR updates when you force-push

@sastan sastan force-pushed the adapter-node-precompress-assets branch from fb3ed4d to bbeaa86 Compare June 15, 2021 20:08
@sastan
Copy link
Contributor Author

sastan commented Jun 15, 2021

Just an FYI – it's much harder to follow/track PR updates when you force-push

Sorry. Don't know what I was thinking. You are totally right.

PS Did the last commit just before I read your comment.

@benmccann
Copy link
Member

@sastan it looks like this PR will need to be rebased

sastan and others added 3 commits June 16, 2021 17:43
sirv supports using precompressed assets but they are not generated during the build. This PR precompresses html, js, json, css, svg, and xml assets using gzip and brotli during the adapt phase which allows sirv to use these.
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@sastan sastan force-pushed the adapter-node-precompress-assets branch from e1112a9 to 37dc6d9 Compare June 16, 2021 15:45
@sastan
Copy link
Contributor Author

sastan commented Jun 16, 2021

@benmccann Rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants