Skip to content

feat: add assets inline exclude#6892

Closed
web-97 wants to merge 3 commits intovitejs:mainfrom
web-97:feat/add-assetsInlineExclude
Closed

feat: add assets inline exclude#6892
web-97 wants to merge 3 commits intovitejs:mainfrom
web-97:feat/add-assetsInlineExclude

Conversation

@web-97
Copy link

@web-97 web-97 commented Feb 13, 2022

Description

Closes #2173

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

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

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy
Copy link
Member

bluwy commented Feb 13, 2022

Closes #2173

@Niputi Niputi changed the title Feat/add assets inline exclude feat: add assets inline exclude Feb 13, 2022
@patak-cat
Copy link
Member

Thanks for the PR!

I think that it would be good to refactor the options to be

  build: {
    assetsInline: {
      limit: 4096,
      include: ...,
      exclude: ...
    }
  }

So we can use the usual include/exclude createFilter pattern.

But let's wait to see what other maintainers think. I'll add it for discussion in our next team meeting (we just had one, so a bit of patience since we are going to get to this one in two weeks).

@patak-cat patak-cat added the p2-nice-to-have Not breaking anything but nice to have (priority) label Feb 13, 2022
@web-97
Copy link
Author

web-97 commented Feb 13, 2022

谢谢你的公关!

我认为将选项重构为

  build: {
    assetsInline: {
      limit: 4096,
      include: ...,
      exclude: ...
    }
  }

所以我们可以使用通常的包含/排除createFilter模式。

但让我们拭目以待,看看其他维护者的想法。我将在下一次团队会议中添加它以供讨论(我们只有一个,所以请耐心等待,因为我们将在两周内完成这个)。

Thanks, looking forward to the next changes

Copy link
Member

@poyoho poyoho left a comment

Choose a reason for hiding this comment

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

Some picky advice 😊

@web-97 web-97 requested a review from poyoho April 18, 2022 17:11
@iChip
Copy link

iChip commented Apr 26, 2022

Any update on this? would love this feature 🙏

@poyoho
Copy link
Member

poyoho commented Apr 27, 2022

emmm... I also think the PR need to #6892 (comment).

@seivan
Copy link

seivan commented Jun 24, 2022

I also have this problem, but I don't think this is a correct solution.
Include/Exclude files patterns are error prone because it lets the user define the same file under the same group.
Also it's a bit confusing, does this mean that if I haven't defined my includes, it will automatically defer to the size rule?
What if I have it under include, but does not fit the size rule?

May I offer an alternative solution
#8717

Make it a callback with the file, filesize, and just for kicks, current total size of everything included so far.
User can choose to whitelist, blacklist, and/or toggle on base64 size or combined inlined sizes so far.

@bluwy
Copy link
Member

bluwy commented Sep 19, 2023

Cleaning up stale PRs. The discussion notes for this favoured the API in #8717 instead with a function form. Closing this for now.

@bluwy bluwy closed this Sep 19, 2023
@web-97 web-97 deleted the feat/add-assetsInlineExclude branch September 23, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p2-nice-to-have Not breaking anything but nice to have (priority)

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Add build.assetInlineExclude config

6 participants