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: module config .format, last entry used for nuxt-image #896

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

hartmut-co-uk
Copy link
Collaborator

@hartmut-co-uk hartmut-co-uk commented Jun 26, 2023

Description

WIth #880 a module level config option format was added which currently applies to <nuxt-picture> only.
This PR extends nuxt-image to use this config parameter as a default/fallback for <nuxt-image>.

TBC

  1. The PR is implemented so that the last element of the array value is used as the default format - is this the right approach?
  2. With current default of ['webp'] all <nuxt-image> are going to be webp format, is this OK? Or should we remove the default value?

References

Solves/completes: #657

Tasks

  • confirm using the last element of the array value is the way to go -> use first item
  • adapt failing tests (module config default is format: ['webp'] -> applies to many tests)
  • keep default ['webp']?

@nuxt-studio
Copy link

nuxt-studio bot commented Jun 26, 2023

Live Preview ready!

Name Edit Preview Latest Commit
Image Edit on Studio ↗︎ View Live Preview 111de6a

@netlify
Copy link

netlify bot commented Jun 26, 2023

Deploy Preview for nuxt-image failed.

Name Link
🔨 Latest commit 111de6a
🔍 Latest deploy log https://app.netlify.com/sites/nuxt-image/deploys/64a704ce60590b000832a08c

@hartmut-co-uk
Copy link
Collaborator Author

@danielroe I've added one new unit test for the change but did not adapt existing tests - waiting for the solution to be discussed/confirmed. Would you have any thoughts on this?

@hartmut-co-uk hartmut-co-uk marked this pull request as draft June 30, 2023 11:00
@hartmut-co-uk hartmut-co-uk requested review from danielroe and removed request for danielroe June 30, 2023 11:00
@danielroe
Copy link
Member

I think the first item in the list is to be preferred for 'modern' formats. I think the only reason we had special handling for the last item in <nuxt-picture> was for legacy format support.

@hartmut-co-uk
Copy link
Collaborator Author

yes and no, <picture> (html) supports multiple formats, so e.g. browsers that do not yet support avif would fallback to webp or the legacy format.
If we pick the first, and if that is avif - then no <nuxt-image> images would be rendered for e.g. Edge / older browsers...

@hartmut-co-uk
Copy link
Collaborator Author

Maybe splitting the default format config between image <> picture would be a better choice?

I have a feeling trying to cover all use cases at the cost of having hidden logic and edge case handling underneath could do more harm than good...

@danielroe
Copy link
Member

The last item in the format array is not the legacy format though. I think if the user is specifying a default format then it makes sense to respect the first one for image as well. It's always overrideable on a per-image basis, and if fallback legacy image formats were important, then you can always use nuxt-picture.

@hartmut-co-uk
Copy link
Collaborator Author

I'm good with this rationale.
So you'd give your thumbs up to:

  1. keep default as-is -> ['webp']
  2. use first item for <nuxt-image>

Then I'll go ahead and complete this PR...

@danielroe
Copy link
Member

That sounds good to me 👍

@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (37395b7) 89.75% compared to head (b91c777) 89.75%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #896   +/-   ##
=======================================
  Coverage   89.75%   89.75%           
=======================================
  Files          44       44           
  Lines        2879     2879           
  Branches      319      320    +1     
=======================================
  Hits         2584     2584           
  Misses        294      294           
  Partials        1        1           
Impacted Files Coverage Δ
src/runtime/components/_base.ts 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hartmut-co-uk hartmut-co-uk marked this pull request as ready for review July 1, 2023 17:01
Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

My main worry here is that this now applies webp default format to all images without user opt-in. For providers that support it, I think auto should be the default, and it should fall back to the default value provided.

So maybe we need to remove the default of webp and apply that as a default value for <nuxt-picture> only.

src/runtime/components/_base.ts Outdated Show resolved Hide resolved
test/unit/image.test.ts Outdated Show resolved Hide resolved
@hartmut-co-uk
Copy link
Collaborator Author

hartmut-co-uk commented Jul 6, 2023

My main worry here is that this now applies webp default format to all images without user opt-in. For providers that support it, I think auto should be the default, and it should fall back to the default value provided.

So maybe we need to remove the default of webp and apply that as a default value for <nuxt-picture> only.

Yes, that's what's been worrying me as well. I just noticed there's an incomplete point (2) in my initial message...
How about removing the default value entirely?

Or would you re-consider

Maybe splitting the default format config between image <> picture would be a better choice?

@danielroe
Copy link
Member

danielroe commented Jul 7, 2023

It's a good point. Maybe support global format (no default value) and then specific picture.format and image.format if that doesn't make it too complicated? At build time, picture and image objects would be merged with generic options (overriding them).

@hartmut-co-uk
Copy link
Collaborator Author

Do we already have global config picture & image objects/types in the meantime?
TBH If we'd go for individually configurable options, I don't see any need for a generic one.

I can see

  • Option A: Stick with what we've had in July, array for picture, use first for image, but drop the default (opt-in)
    image: {
      format: ['webp'] // undefined by default
    }
  • Option B: Make format an object, with fields for image vs. picture
    image: {
      format: {
        image: 'auto',
        picture: ['webp']
      }
    }
  • Option C: Individual config objects for image vs. picture
    image: {
      image: {
        format: 'auto',
      },
      picture: {
        format: ['webp']
      }
    }
  • Option D: Individual config fields image vs. picture
    image: {
      imageFormat: 'auto',
      pictureFormat: ['webp']
    }

I find (B) confusing, (C) very confusing. I could live with (A), unsure but (D) might be OK?

@danielroe danielroe added the enhancement New feature or request label Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants