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

Add limitInputPixels option for sharp image service #9546

Merged
merged 8 commits into from
Jan 3, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Dec 29, 2023

Changes

close #8886

Testing

Tested manually

Docs

The documentation doesn't quite mention how options are passed. I suppose we should add one? cc @withastro/maintainers-docs

Copy link

changeset-bot bot commented Dec 29, 2023

🦋 Changeset detected

Latest commit: b316915

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 pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Dec 29, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@Princesseuh Princesseuh added this to the 4.1.0 milestone Jan 2, 2024
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 @bluwy! This one is going to require a little bit of figuring out, but I'll turn up early tomorrow in case anything doesn't feel obvious!

'astro': minor
---

Adds `limitInputPixels` option for the Sharp image service to be passed on to Sharp.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Adds `limitInputPixels` option for the Sharp image service to be passed on to Sharp.
Adds an option for the Sharp image service to allow large images to be processed. Set `limitInputPixels: false` to bypass the default image size limit:
```
// astro.mjs.config
example showing this config
```

(I'm assuming this is a config-wide option set, and not e.g. per-image.)

Depending on the appropriate code sample here, the same will need to be added to the astro types file. Since sharp is the default image service, someone might not have this in their config already, but they'd need to add it in order to pass the option. So, I'm thinking somewhere here: https://docs.astro.build/en/reference/configuration-reference/#imageservice but it would have to account for the fact that someone is only adding this NOT to actually change the image service used, but rather to configure the default one. (Since, this option is JUST for Sharp, and not the other services)

Depending on whatever's decided here, for an actual docs PR, we can even get away with just linking to the config option from here: https://docs.astro.build/en/guides/images/#default-image-service and don't need to explain it. We really should have a link there anyway! 😅

Sorry this requires a bit of figuring out, but I'll make sure I get up early tomorrow and am happy to help with it!

Copy link
Member Author

@bluwy bluwy Jan 3, 2024

Choose a reason for hiding this comment

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

Thanks for the review! Yeah I have a rough idea to improve this now.

Thanks for linking me to the docs. I actually would've documented something like:

import { defineConfig, sharpImageService } from 'astro/config'

export default defineConfig({
  image: {
    service: sharpImageService({ limitInputPixels: false })
  }
})

But I think it's easier to keep the object format for now, so it applies more generally to other kinds of image services.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the changesets and jsdoc. Let me know what you think!

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.

Looks great, @bluwy ! Thank you!

@ematipico ematipico merged commit 08402ad into main Jan 3, 2024
14 checks passed
@ematipico ematipico deleted the sharp-limit-input-pixels branch January 3, 2024 13:19
@astrobot-houston astrobot-houston mentioned this pull request Jan 3, 2024
@jlarmstrongiv
Copy link

jlarmstrongiv commented Jan 4, 2024

It seems that the release notes

// astro.config.mjs
import { defineConfig } from 'astro/config';

export default defineConfig({
  image: {
    service: {
      entrypoint: 'astro/assets/services/sharp',
      config: {
        limitInputPixels: false,
      },
    },
  },
});

and config

export interface SharpImageServiceConfig {
    /**
     * The `limitInputPixels` option passed to Sharp. See https://sharp.pixelplumbing.com/api-constructor for more information
     */
    limitInputPixels?: number;
}

Have different option types, though the type should match options.limitInputPixels https://sharp.pixelplumbing.com/api-constructor

For now, setting limitInputPixels: 0 is the same as limitInputPixels: false, but without the type error

@bluwy
Copy link
Member Author

bluwy commented Jan 5, 2024

@jlarmstrongiv you're right! I mistyped that. Will send a followup PR to fix it

@bluwy bluwy mentioned this pull request Jan 5, 2024
kdheepak added a commit to ratatui/ratatui-website that referenced this pull request Feb 21, 2024
This change is required because some gifs were too big to be preprocessed and causes errors during build:

withastro/astro#9546
kdheepak added a commit to ratatui/ratatui-website that referenced this pull request Feb 21, 2024
This change is required because some gifs were too big to be
preprocessed and causes errors during build:

withastro/astro#9546
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) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alllow setting limitInputPixels with Sharp
5 participants