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: add assets inline exclude #6892

Closed
wants to merge 3 commits into from
Closed

feat: add assets inline exclude #6892

wants to merge 3 commits into from

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-dev
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-dev patak-dev 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 😊

packages/vite/src/node/plugins/asset.ts Show resolved Hide resolved
docs/config/index.md Show resolved Hide resolved
docs/config/index.md Show resolved Hide resolved
@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