-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Warn when broad glob patterns are used in the content configuration #14140
Conversation
We will show a warning if all of these conditions are true: 1. We detect `**` in the glob pattern 2. _and_ you didn't explicitly use `node_modules` in the glob pattern 3. _and_ we found files that include `node_modules` in the file path 4. _and_ no other globs exist that explicitly match the found file With these rules in place, the DX has nice trade-offs: 1. Very simple projects (that don't even have a `node_modules` folder), can simply use `./**/*` because while resolving actual files we won't see files from `node_modules` and thus won't warn. 2. If you use `./src/**` and you do have a `node_modules`, then we also won't complain (unless you have a `node_modules` folder in the `./src` folder). 3. If you work with a 3rd party library that you want to make changes to. Using an explicit match like `./node_modules/my-package/**/*` is allowed because `node_modules` is explicitly mentioned. Note: this only shows a warning, it does not stop the process entirely. The warning will be show when the very first file in the `node_modules` is detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to make builds so much faster for people that accidentally have these patterns in their config. Awesome! I left some input on the warning text
Accidentally removed a line
+ use the actual glob/file that is causing issues
// Ensures that `node_modules` has to match as-is, otherwise `mynode_modules` | ||
// would match as well, but that is not a known large directory. | ||
const LARGE_DIRECTORIES_REGEX = new RegExp( | ||
`(${LARGE_DIRECTORIES.map((dir) => String.raw`\b${dir}\b`).join('|')})` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use dir separators here? e.g. [/\\]${dir}[/\\]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good question, I used \b
to not have to deal with path separators at all haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good to me 👍
Co-authored-by: Adam Wathan <adam.wathan@gmail.com>
I think that this feature has false positives. This is the warning I get:
I may be wrong, but I don't think that By the way, this pattern is from the Laravel Jetstream default template. |
@philipp-spiess you're right, my project has a |
Just running into the same issue @LucaRed mentioned, not just Laravel Jetstream, also following the installation guide would trigger the warning; https://tailwindcss.com/docs/guides/laravel
/** @type {import('tailwindcss').Config} */
export default {
content: [
+ "./resources/**/*.blade.php",
"./resources/**/*.js",
"./resources/**/*.vue",
],
theme: {
extend: {},
},
plugins: [],
} |
After shipping the new warning that prevents unexpected scanning of all dependencies in 3.4.8, we noticed that it was firing more often than we wanted to. The heuristics we added works by finding broad glob patterns (once that contain `/**/`) and when those are found and are the _sole pattern used to match a file of a known-large directory_, we were showing the warning. The motivation for this is that we have seen time and time again that an incorrect config like `/**/*.js` can cause recursive scans through _all_ dependencies including many minified libraries which greatly impacts performance. In #14140, we were adding two known-large directory names: - `node_modules` (used by npm) - `vendor` (used by PHP) The problem with the `vendor` name though is that it is more generic than we would like it and there are legit use cases to have a folder named `vendor` inside your component folder. Additionally, PHP vendors behave a bit differently and it's not super common to have minified build files in that folder (which is one of the main reasons for the slow builds). Because of this, we decided to revert the change for `vendor` and only scan for `node_modules` going forward.
When you use a glob pattern in your
content
configuration that is too broad, it could be that you are accidentally including files that you didn't intend to include. E.g.: all ofnode_modules
This has been documented in the Tailwind CSS documentation, but it's still something that a lot of people run into.
This PR will try to detect those patterns and show a big warning to let you know if you may have done something wrong.
We will show a warning if all of these conditions are true:
**
in the glob patternnode_modules
in the glob patternnode_modules
in the file pathWith these rules in place, the DX has nice trade-offs:
node_modules
folder), can simply use./**/*
because while resolving actual files we won't see files fromnode_modules
and thus won't warn../src/**
and you do have anode_modules
, then we also won't complain (unless you have anode_modules
folder in the./src
folder)../node_modules/my-package/**/*
is allowed becausenode_modules
is explicitly mentioned.Note: this only shows a warning, it does not stop the process entirely. The warning will be show when the very first file in the
node_modules
is detected.