xzyfer / sass-graph

Parses import dependencies from a directory of sass files
MIT License
76 stars 59 forks source link

Add option to exclude files based on regular expression #86

Closed MarcelRobitaille closed 6 years ago

MarcelRobitaille commented 7 years ago

I wanted to exclude /node_modules/.

MarcelRobitaille commented 7 years ago

Is this likely to get merged or should I publish a fork?

xzyfer commented 7 years ago

Exclusions are likely to be merged. Globbing will not.

On 27 Jun. 2017 12:21 pm, "Marcel Robitaille" notifications@github.com wrote:

Is this likely to get merged or should I publish a fork?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/xzyfer/sass-graph/pull/86#issuecomment-311234152, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWPYFX8WoBN7gtCsanmnzJrXy7_2Zks5sIGdEgaJpZM4OClFb .

MarcelRobitaille commented 7 years ago

Do you mind if I publish my fork on npm then?

xzyfer commented 7 years ago

If you wish :)

xzyfer commented 7 years ago

FWIW this implementation isn't complete. It's important to also do the exclusion when determine the ancestors and descendants. You'd also probably want to cache the regex check in the constructor to avoid doing it many times.

MarcelRobitaille commented 7 years ago

I didn't want to check ancestors and descendants because I assumed the usual use case would be /node_modules/ and I wanted to keep things simple. To add the check to ancestors and descendants would it just be as simple as doing the same check in Graph.prototype.visitAncestors and Graph.prototype.visitDescendents?

What do you mean by caching the regex check? You mean this.exclude instanceof RegExp?

MarcelRobitaille commented 7 years ago

Something like that?

xzyfer commented 7 years ago

Not dealing with ancestors and descendants will mean that if a file imports a file from node_modules is appear in the dependency graph. This may be your intention but it's "surprising" and breaks the primary usecase of this library - the node-sass watcher.

xzyfer commented 7 years ago

If files and/or directories are being excluded there should be no way for those exclude to appear in the graph.

MarcelRobitaille commented 7 years ago

That makes sense. Is what I suggested the best way to do that?

xzyfer commented 7 years ago

The regex caching is better. You'll still need to prevent exclude files being referenced in as ancestors and descendants.

MarcelRobitaille commented 7 years ago

I'll write some tests I guess.

xzyfer commented 7 years ago

Please remove package-lock.json

MarcelRobitaille commented 7 years ago

Sure thing. BTW you should have write access, no?

xzyfer commented 7 years ago

I do but I'm mostly catching up whilst in bed. Open source isn't my day job :)

xzyfer commented 7 years ago

This is looking good, thanks for your time.

I have some performance concerns, but they can be address separately. I'll get this released in the next day or so.

MarcelRobitaille commented 7 years ago

Yeah it's true that it checks a lot of regexes but idk how to avoid that.

MarcelRobitaille commented 7 years ago

Any update on this?

MarcelRobitaille commented 7 years ago

What are we waiting on? I'd like to make my fork after this has been merged.

xzyfer commented 6 years ago

Thanks @Iambecomeroot. Apologies for the delay I've be occupied with other projects.