Skip to content

Commit

Permalink
Use "with" instead of regex after lemonzi's recommendation
Browse files Browse the repository at this point in the history
  • Loading branch information
danielstjules committed Mar 5, 2015
1 parent 71d72d4 commit 508c4e6
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 53 deletions.
20 changes: 12 additions & 8 deletions lib/pjs.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,14 @@ var _nullObject = utils._nullObject;
* returns {Transform} A transform stream in objectMode
*/
exports.filter = function(expression, outputString, explicit) {
var exp, i, include, filterStream;
var i, include, filterStream;

exp = (explicit) ? expression : utils.getBoundExpression(expression, '$');
i = 0;

include = function($, i) {
return eval(exp);
with ($) {
return eval(expression);
}
};

filterStream = new stream.Transform({objectMode: true});
Expand Down Expand Up @@ -58,19 +59,22 @@ exports.filter = function(expression, outputString, explicit) {
* returns {Transform} A transform stream in objectMode
*/
exports.map = function(expression, outputString, explicit) {
var exp, i, update, mapStream;
var i, update, mapStream;

exp = (explicit) ? expression : utils.getBoundExpression(expression, '$');
i = 0;

update = function($, i) {
var _result = eval('(' + exp + ')');
return (_result === null) ? _nullObject : _result;
with ($) {
var _result = eval('(' + expression + ')');
return (_result === null) ? _nullObject : _result;
}
};

if (outputString) {
update = function($, i) {
return String(eval('(' + exp + ')')) + "\n";
with ($) {
return String(eval('(' + expression + ')')) + "\n";
}
};
}

Expand Down
19 changes: 0 additions & 19 deletions lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,22 +123,3 @@ utils.concat = function(array) {
return String(curr) + String(prev);
});
};

/**
* Given a string expression to be evaluated, binds any String properties and
* methods to the provided variable. Returns the resulting string.
*
* @param {string} expression The expression to bind
* @param {string} varName Variable to which to bind
*/
utils.getBoundExpression = function(expression, varName) {
Object.getOwnPropertyNames(String.prototype).forEach(function(key) {
var regexStr, pattern;

regexStr = '([^\\w\\.\\{]\\s*|^\\s*)(' + key + ')([^\\w\\.]|$)';
pattern = new RegExp(regexStr, 'g');
expression = expression.replace(pattern, "$1" + varName + ".$2$3");
});

return expression;
};
26 changes: 0 additions & 26 deletions spec/utilsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,30 +84,4 @@ describe('utils', function() {
expect(result).to.be('foobarnull');
});
});

describe('getBoundExpression', function() {
it('prepends identifiers that are keys in String.prototype by the var', function() {
var result = utils.getBoundExpression('trim()', '$');
expect(result).to.be('$.trim()');
});

it('correctly attributes identifiers given leading whitespace', function() {
var result = utils.getBoundExpression(' trim()', '$');
expect(result).to.be(' $.trim()');
});

it('does not modify keys in object literals', function() {
var result = utils.getBoundExpression('{length: length}', '$');
expect(result).to.be('{length: $.length}');
});

it('ignores identifiers in which a prototype key is a substring', function() {
var identifiers = ['asubstr', 'substra', 'asubstra', '_substr',
'substr_', '_substr_', 'substr123', 'x.substr'];

identifiers.forEach(function(str) {
expect(utils.getBoundExpression(str, '$')).to.be(str);
});
});
});
});

4 comments on commit 508c4e6

@lemonzi
Copy link
Contributor

@lemonzi lemonzi commented on 508c4e6 Mar 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I had for the PR :)

I kept explicit so the user could disable the binding to keep the API compliant, but I'm not sure it's any useful.

@danielstjules
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we haven't released a stable major yet, I think it's fine if we drop a flag in a new minor release. Especially since it isn't particularly useful anymore :)

@lemonzi
Copy link
Contributor

@lemonzi lemonzi commented on 508c4e6 Mar 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great then :)

@danielstjules
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! Done in 4af10a5 Published a new version on npm.

Please sign in to comment.