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

Warn when broad glob patterns are used in the content configuration #14140

Merged
merged 12 commits into from
Aug 7, 2024

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Aug 7, 2024

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 of node_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:

  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.

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.
@RobinMalfait RobinMalfait changed the title Show warning when detecting broad glob patterns Warn when broad glob patterns are used in the content configuration Aug 7, 2024
Copy link
Contributor

@philipp-spiess philipp-spiess left a 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

CHANGELOG.md Show resolved Hide resolved
src/cli/build/plugin.js Outdated Show resolved Hide resolved
src/lib/content.js Show resolved Hide resolved
// 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('|')})`
Copy link
Contributor

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}[/\\]?

Copy link
Member Author

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.

Copy link
Contributor

@thecrypticace thecrypticace left a 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 👍

src/lib/content.js Outdated Show resolved Hide resolved
@RobinMalfait RobinMalfait merged commit 858696a into master Aug 7, 2024
13 checks passed
@RobinMalfait RobinMalfait deleted the feat/warn-on-greedy-globs branch August 7, 2024 14:55
@LucaRed
Copy link

LucaRed commented Aug 8, 2024

I think that this feature has false positives. This is the warning I get:

warn - Your `content` configuration includes a pattern which looks like it's accidentally matching all of `vendor` and can cause serious performance issues.
warn - Pattern: `./resources/views/**/*.blade.php`
warn - See our documentation for recommendations:
warn - https://tailwindcss.com/docs/content-configuration#pattern-recommendations

I may be wrong, but I don't think that ./resources/views/**/*.blade.php matches anything inside of the vendor folder.

By the way, this pattern is from the Laravel Jetstream default template.

@philipp-spiess
Copy link
Contributor

@LucaRed Any chance that your views folder has a vendor subfolder? There are others with similar issues and we're already thinking about how we can improve this heuristics: #14145 Sorry for the trouble!

@LucaRed
Copy link

LucaRed commented Aug 8, 2024

@philipp-spiess you're right, my project has a /resources/views/vendor folder because I need to override some package views.

@RobertBoes
Copy link
Contributor

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

Configure your template paths
Add the paths to all of your template files in your tailwind.config.js file.

/** @type {import('tailwindcss').Config} */
export default {
  content: [
+    "./resources/**/*.blade.php",
    "./resources/**/*.js",
    "./resources/**/*.vue",
  ],
  theme: {
    extend: {},
  },
  plugins: [],
}

RobinMalfait pushed a commit that referenced this pull request Aug 8, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants