robrich / gulp-match

Does a vinyl file match a condition?
MIT License
23 stars 9 forks source link

gulp-match always returns true for string condition in Node v6.0.0 #12

Closed tony19 closed 8 years ago

tony19 commented 8 years ago

gulp-match converts a condition string (a filename pattern) into a RegExp in:

if (typeof condition === 'string' && condition.match(/^\*\.[a-z\.]+$/)) {
    var newCond = condition.substring(1).replace(/\./g,'\\.')+'$';
    condition = new RegExp(newCond);
}

...and then it tries to verify that the condition object is a RegExp (e.g., caller passed in a RegExp):

if (typeof condition === 'object' && typeof condition.test === 'function' && condition.hasOwnProperty('source')) {
    // FRAGILE: ASSUME: it's a regex
    return condition.test(file.relative);
}

The hasOwnProperty-check above works in Node 5 and earlier but fails in Node 6 for some reason, which leads to gulp-match returning !!condition at the end of the function, which is always true.

A potential workaround is to replace that check with typeof:

if (typeof condition === 'object' && typeof condition.test === 'function' && typeof condition.source !== 'undefined') {
    ...
}
robrich commented 8 years ago

Thanks for the detail. Would you like to make a pull request with tests that flex this in node 6 and this patch to solve it?

tony19 commented 8 years ago

Ok, done (#13).

I can't explain why hasOwnProperty('source') is failing for RegExp in Node 6 though. It has me curious about the other hasOwnProperty calls in gulp-match. Any thoughts on that?

nborko commented 8 years ago

I just got hit by this. FWIW, the equivalent test would be Object.getPrototypeOf(condition).hasOwnProperty('source')

tony19 commented 8 years ago

@nborko Ah, that works as well. Why is it needed in Node 6?

nborko commented 8 years ago

@tony19 It's probably dependent on the V8 version used in Node 6. In ES5 source was a straight up data property of the RegExp object. In ES6 it's a prototype accessor property. Migrating from Node 4 to Node 6 is (apparently) 93% migrating from ES5 to ES6, and there are lots of these little surprises in waiting :)