Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Filters always ignore dot-files #9

Closed
IvanSanchez opened this issue Aug 10, 2016 · 8 comments · Fixed by #62
Closed

Filters always ignore dot-files #9

IvanSanchez opened this issue Aug 10, 2016 · 8 comments · Fixed by #62

Comments

@IvanSanchez
Copy link

Scenario: GobbleJS workflow with merge nodes, and a rollup transform with some rollup plugins, like:

var code = gobble([txt, js])
.transform('rollup', {
    entry: 'main.js',
    dest: 'dist/main.js',
    format: 'cjs',
    sourceMap: true,
    plugins: [
        require('rollup-plugin-string')({ include: '**/*.txt' }),
        require('rollup-plugin-buble')({ exclude: '**/*.txt'}),
        require('rollup-plugin-commonjs')(),
        require('rollup-plugin-node-resolve')()
    ]
});

Due to Gobble's merge nodes, the files are in a path containing the hidden subdirectory name .gobble, like:

/home/ivan/devel/project/.gobble/03-merge/1/src/submodule/messages.txt

This was throwing errors in the buble transform, as apparently the *.txt files were being processed by rollup-plugin-buble, where I wanted them to be processed just by rollup-plugin-string.

Fortunately, I've been able to track down the problem to createFilter in the rollup-pluginutils module.

See, createFilter uses minimatch, and looking at its documentation I found this bit:

All options are false by default.
[…]
dot
Allow patterns to match filenames starting with a period, even if the pattern does not explicitly have a period in that spot.
Note that by default, a/**/b will not match a/.d/b, unless dot is set.

This means that all filters for all rollup plugins are broken when rollup is used within gobble (not sure about whether other build tools use paths with dot-files for temp/hidden files).

I think I can work around the issue by specifying minimatch patterns with an explicit .gobble/**/ for every rollup plugin, but this is totally counter-intuitive.

So:

  • What would be a sane default for the dot setting of minimatch?
  • Should createFilter accept an extra set of minimach options?
  • How can we make gobble-rollup force dot=true in the rollup plugin filters?
@Rich-Harris
Copy link
Contributor

FYI this may be fixed as of version 2.0.0, which uses micromatch instead of minimatch

@tunnckoCore
Copy link

tunnckoCore commented Jan 3, 2017

@Rich-Harris huh, my mistake. It won't be fixed, because that the default in micromatch too. But the fix will be to allow passing options directly to micromatch.

edit: it also has ignore option, so you can pass exclude to it directly.
edit2: or at least, provide options object as third argument would be the easiest fix

@tunnckoCore
Copy link

tunnckoCore commented Jan 3, 2017

I believe we could implement createFilter a lot better. Remove L6-L8 lines and L15-L27 and just use main export micromatch()

import micromatch from 'micromatch'

const createFilter = (include, exclude, options) => (id) => {
  if ( typeof id !== 'string' ) return false;
  if ( /\0/.test( id ) ) return false;

  options = Object.assign({ ignore: exclude }, options)

  return micromatch(id, include, options).length > 0
}

a lot simpler and maintainable and customizable. But yea drop support for regexps, but it can be worked around too.

edit: It's not only simpler and smaller, but micromatch (actually v3) handle normalization of windows paths and etc stuff, don't see sense for this id = id.split( sep ).join( '/' );. Actually it's not a problem to use micromatch() it is pretty optimized as possible.

@tunnckoCore
Copy link

tunnckoCore commented Jan 28, 2017

@Rich-Harris ping?

edit: i'm testing it, can confirm later.

@mrkishi
Copy link

mrkishi commented May 17, 2019

include: ['node_modules/**'] will also not match dependencies installed by pnpm, since they end up inside node_modules/.registry.npmjs.org/.

Are there reasons not to default to dot: true? I'd expected include: ['**'] and import stuff from '.stuff' to match.

@lukastaegert
Copy link
Member

I guess there really is no reason to exclude them. Will see if I can prepare a small patch.

@lukastaegert
Copy link
Member

Fix at #62

@sntran
Copy link

sntran commented Aug 1, 2019

Sorry for reopening an old issue. The fix at #62 does not handle the case of ./stuff/file.ext.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants