[pluginutils] strange behavior for include/exclude pattern

[LarsDenBakker]

• Rollup Plugin Name: pluginutils
• Rollup Plugin Version: 3.1.0

Expected Behavior / Situation

Include/exclude patterns work in an intuitive way

Actual Behavior / Situation

Include/exclude patterns don't work in an intuitive way

Modification Proposal

I spent quite some time trying to debug why my include/exclude patterns were not working correctly. Then I found out that here we are doing some magic by prefixing the current working directory to the user provided patterns: https://github.com/rollup/plugins/blob/master/packages/pluginutils/src/createFilter.ts#L15

This creates an invalid pattern when the pattern also includes the working directory. For example I was trying to pass on an explicit list of files to exclude from a plugin. The current working directory got prepended to that, resulting in a pattern that would never match.

For example /Users/me/some/path/to/code.js becomes /Users/me/some/path/Users/me/some/path/to/code.js.

Another problem I ran into is that this way it's impossible to match any files outside of the current working directory. I'm running rollup from a package in a monorepo, the node modules folder is actually two directories upwards.

We could add some magic to detect when to resolve patterns, but I'm wondering why it would be necessary. Could we not just follow the minimatch library logic here?

[shellscape]

There's historical reasons for the way it works atm. I had the same questions working on that lib a while back. I can't recall what exactly the reasoning was, but I do remember I found it at the old repo. Follow the git blame road here https://github.com/rollup/rollup-pluginutils

[lukastaegert]

That is what the resolve option exists for: https://github.com/rollup/plugins/blob/master/packages/pluginutils/README.md#resolve

[LarsDenBakker]

The resolve option isn't exposed from most plugins. The tough part with match patterns is that you don't really know if something is wrong. You set up a pattern and expect it to "just work". So even if we add an option, I don't think a lot of people would use it because they don't realize something is wrong.

For example if my current working directory is /Users/me/foo/bar and I use a plugin like this:

export default {
  plugins: [
    myPlugin({ include: ['**/*.css'], exclude: ['/Users/me/foo/node_modules/some-project/some-file.css'] })
  ]
}

The **/*.css pattern gets resolved to something like /Users/me/foo/bar/**/*.css and /Users/me/foo/node_modules/some-project/some-file.css gets resolved to /Users/me/foo/bar /Users/me/foo/node_modules/some-project/some-file.css.

I expect this to match all CSS files in the build, but in fact it's only matching CSS files under /Users/me/foo/bar and not using my exclude at all.

From the history I couldn't tell why the patterns are resolved, only that the resolve option was added to address this problem. Ideally resolve should default to false, with an option to opt into it. But it's hard to judge whether anyone relies on the resolve behavior. We could avoid resolving patterns that start with a /, although that's still something people would need to learn about.

[lukastaegert]

I think the behaviour is meant to match what Rollup itself is doing. E.g. if you set input: "main.js" in your config file, Rollup is looking for that file in process.cwd(), so it makes sense that you should also be able to put "main.js" into an include/exclude pattern in the same file.

We could avoid resolving patterns that start with a /

Feels like a very good idea to me, could be even considered a bug fix, at least I do not see where this would be breaking. Though I guess it should also be made to work for absolute windows paths.

[LarsDenBakker]

Rollup itself doesn't support globs though, but I understand what the intention is.

We could not add it for / and it's windows equivalent, and teach people to add / in front of patterns like /**/node_modules/**/*.

Still, it's confusing to have different behavior than other matching libraries. Maybe we could also not add the current working directory to patterns starting with *, or perhaps there is some heuristic to determine if a path is a relative path and only add it for those cases.

Alternatively there could be a separate includePattern / excludePattern option, though that would not be ideal.

[CharlesMoone]

my project:

lib
- index.js
- class
-- a.js
-- b.js

third-part package like 'xxx'.

I want to use babel to translate all my file and the third-party package 'xxx':

include: ['lib', 'node_modules/xxx'] ❌
include: ['lib/*', 'node_modules/xxx'] ❌
include: ['lib/*.js', 'node_modules/xxx'] ❌

finnaly
babel({ include: ['lib/*.js', 'lib/**/*.js'] }); ✔️

sry, third-party still fail......

[mjackson]

I ran into this same issue today which really confused me for a while. I am trying to use an absolute path as my include value which works with minimatch, but doesn't work with createFilter.

// createFilter doesn't match...
f = createFilter('/a/b/**/*')
f('/a/b/c.js') // false

// but minimatch does! :D 
minimatch.match(['/a/b/c.js'], '/a/b/**/*') // ['/a/b/c.js']

IMO this could be fixed by adding support for absolute paths in include and exclude, so I'm 👍 on adding support for a leading /.

The workaround for me for now involves using path.relative just so I can get a relative path that will then be resolved correctly...