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: expose createFilter util #8562

Merged
merged 3 commits into from Jun 13, 2022
Merged

feat: expose createFilter util #8562

merged 3 commits into from Jun 13, 2022

Conversation

patak-dev
Copy link
Member

Description

We are already exposing normalizePath, another @rollup/pluginutils function. createFilter is used internally and in the plugins in the monorepo. Exposing it in Vite may encourage more people to follow the recommended include/exclude pattern.

Reference discussion with @sheremet-va for Vitest: vitest-dev/vitest#1467 (comment)

@antfu I needed to bump the size limit of the CJS build. I tried to avoid exposing createFilter in publicUtils but we are ourselves using the CJS build for plugins internally. Do you think the extra 50kb is an issue here? The other option is to avoid its use in our plugins for the moment.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@@ -44,7 +44,6 @@
"@babel/plugin-transform-react-jsx-development": "^7.16.7",
"@babel/plugin-transform-react-jsx-self": "^7.17.12",
"@babel/plugin-transform-react-jsx-source": "^7.16.7",
"@rollup/pluginutils": "^4.2.1",
"react-refresh": "^0.13.0"
},
"peerDependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

In case, Every plugin using needs to bump peerDeps after Vite's release.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are already requiring this for all internal plugins for v3, no? If not, yes, we should.

Copy link
Member

Choose a reason for hiding this comment

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

We are requiring ^3.0.0-alpha which means 3.0.0-alpha.1 can be used.
But now we need to require ^3.0.0-alpha.11 (next version). (since createFilter is not exported in Vite 3.0.0-alpha.10, current version)

@sapphi-red
Copy link
Member

The CJS build size is bigger mainly because of picomatch (it's 51.86kB). picomatch has no dependency and it's only 88.7kB. Maybe we could externalize picomatch. That will reduce the duplicate between CJS and ESM build.

dist size (picomatch not externalized): 3.21MB
dist size (picomatch externalized): 3.11 MB ( + 88.7kB)

@patak-dev
Copy link
Member Author

Given that we only have 4 deps externalized right now, I would prefer to avoid externalizing picomatch. If we want to avoid duplication, maybe we can avoid exposing it in CJS compat.

@sapphi-red
Copy link
Member

IMO it is possible to have an extra 50kB for this. 👍

@patak-dev patak-dev requested a review from antfu June 13, 2022 14:23
@bluwy
Copy link
Member

bluwy commented Jun 13, 2022

Giving a +1 as it'd help remove the @rollup/pluginutil dep for vite-plugin-svelte too

antfu
antfu approved these changes Jun 13, 2022
@patak-dev patak-dev merged commit c5c424a into main Jun 13, 2022
9 of 18 checks passed
@patak-dev patak-dev deleted the feat/expose-create-filter branch June 13, 2022 14:52
@patak-dev patak-dev added the p2-nice-to-have 🍰 Not breaking anything but nice to have (priority) label Jun 13, 2022
kazuma1989 added a commit to kazuma1989/storybook-builder-vite that referenced this pull request Aug 1, 2022
because `createFilter` util is exposed from Vite v3.
vitejs/vite#8562
kazuma1989 added a commit to kazuma1989/storybook-builder-vite that referenced this pull request Aug 1, 2022
because `createFilter` util is exposed from Vite v3.
vitejs/vite#8562
IanVS pushed a commit to storybookjs/builder-vite that referenced this pull request Aug 8, 2022
because `createFilter` util is exposed from Vite v3.
vitejs/vite#8562
@@ -208,7 +208,7 @@ function createCjsConfig(isProduction: boolean) {
...Object.keys(pkg.dependencies),
...(isProduction ? [] : Object.keys(pkg.devDependencies))
],
plugins: [...createNodePlugins(false, false, false), bundleSizeLimit(55)]
plugins: [...createNodePlugins(false, false, false), bundleSizeLimit(120)]
Copy link
Member

Choose a reason for hiding this comment

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

Quick question: How much leeway do you give the bundle size limit?

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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants