Support nested function #119

Merged
merged 1 commit into from Dec 5, 2013

Conversation

Projects
None yet
3 participants
Contributor

bolasblack commented Nov 25, 2013

No description provided.

@jonathanong jonathanong and 1 other commented on an outdated diff Nov 25, 2013

lib/plugins/function.js
@@ -8,16 +8,25 @@ var utils = require('../utils');
var strip = utils.stripQuotes;
/**
+ * Store all defined functions
+ */
+
+var functions = {};
@jonathanong

jonathanong Nov 25, 2013

Contributor

functions shouldn't be global. or why did you make them global?

@bolasblack

bolasblack Nov 25, 2013

Contributor

@jonathanong When we using rework, we can call .functions with multiple time, then some time will appear this case:

JavaScript:

css.use(rework.function({
  func-in-first-call: function(arg){ return arg; },
  func2-in-first-call: function(arg){ return arg; }
}).use(rework.function({
  func-in-second-call: function(arg){ return arg; }
});

CSS:

func-in-first-call(func-in-second-call(func2-in-first-call(10px)))
/* output */
func-in-first-call(10px)

You can see the unit test:

bolasblack/rework@5de6d82#diff-fdacade56a5f218dd71703fab45cbd7aR221

@jonathanong

jonathanong Nov 25, 2013

Contributor

i'm not sure this is the best way to solve this problem, though, since with globals you can potentially create unintended bugs. why call the middleware twice? why not just call it once with a bigger object?

@bolasblack

bolasblack Nov 25, 2013

Contributor

I think you are right. I made a stupid mistake.

@jonathanong

jonathanong Nov 25, 2013

Contributor

hopefully you're just fixing that part then. i think this PR is still very useful!

@bolasblack

bolasblack Nov 25, 2013

Contributor

@jonathanong thanks for your encourage

bolasblack closed this Nov 25, 2013

bolasblack reopened this Nov 25, 2013

Contributor

bolasblack commented Nov 25, 2013

@jonathanong fixed now

@jonathanong jonathanong and 1 other commented on an outdated diff Dec 2, 2013

lib/plugins/function.js
- args = args.split(/,\s*/).map(strip);
- return func.apply(decl, args);
- });
+ var generatedFuncs = [];
+
+ while (decl.value.match(matcher)) {
+ decl.value = decl.value.replace(matcher, function(_, name, args){
+ var result;
+ if (!parseArgs) {
+ result = funcs[name].call(decl, strip(args));
+ } else {
+ args = args.split(/\s*,\s*/).map(strip);
+ result = funcs[name].apply(decl, args);
+ }
+ // Prevent the case function `rgba` return string contain `rgba(...)`
+ generatedFunc = {from: name, to: name + Date.now() + ''};
@jonathanong

jonathanong Dec 2, 2013

Contributor

not 100% sure what's going on here. maybe it just needs more comments. why would you need the current date for anything?

@bolasblack

bolasblack Dec 2, 2013

Contributor

User may define a function like this:

var prefix = '/some/prefix/path'

return {
  url: function(path) {
    return 'url(' +prefix + path + ')'
  }
}

Then the program will fall into infinite loop...

@jonathanong

jonathanong Dec 2, 2013

Contributor

OH. you should add a comment and a test case for that. that's a good catch.

@bolasblack

bolasblack Dec 2, 2013

Contributor

OK 👌

Contributor

bolasblack commented Dec 2, 2013

@jonathanong OK now 😆

@jonathanong jonathanong commented on an outdated diff Dec 3, 2013

lib/plugins/function.js
if (false !== parseArgs) parseArgs = true;
- var regexp = new RegExp(escape(name) + '\\(([^\)]+)\\)', 'g');
+ var matcher = matcherBuilder(Object.keys(funcs).join('|'));
@jonathanong

jonathanong Dec 3, 2013

Contributor

this should be in module.exports instead of being defined on every declaration

@jonathanong jonathanong and 1 other commented on an outdated diff Dec 3, 2013

lib/plugins/function.js
declarations.forEach(function(decl){
if ('comment' == decl.type) return;
- if (!~decl.value.indexOf(name + '(')) return;
- decl.value = decl.value.replace(regexp, function(_, args){
- if (!parseArgs) return func.call(decl, strip(args));
- args = args.split(/,\s*/).map(strip);
- return func.apply(decl, args);
- });
+ var generatedFuncs = [];
+
+ while (decl.value.match(matcher)) {
+ decl.value = decl.value.replace(matcher, function(_, name, args){
+ // Ensure result is string
+ var result = '';
@jonathanong

jonathanong Dec 3, 2013

Contributor

why would result not be a string? i would just do var result; here and result = instead of += down below

@bolasblack

bolasblack Dec 3, 2013

Contributor

@jonathanong �Because some function will return a number or other type value, but i need use result.replace in line 72.

So i need ensure result is a string.

@jonathanong jonathanong and 1 other commented on an outdated diff Dec 3, 2013

lib/plugins/function.js
+ if (!parseArgs) {
+ result += funcs[name].call(decl, strip(args));
+ } else {
+ args = args.split(/\s*,\s*/).map(strip);
+ result += funcs[name].apply(decl, args);
+ }
+
+ // Prevent fall into infinite loop like this:
+ //
+ // {
+ // url: function(path) {
+ // return 'url(' + '/some/prefix' + path + ')'
+ // }
+ // }
+ //
+ generatedFunc = {from: name, to: name + Date.now()};
@jonathanong

jonathanong Dec 3, 2013

Contributor

not really comfortable using Date.now() here though since there's potential for conflicts. can we do a really silly uid or something? my favorite: Math.random().toString(36).slice(2)

@bolasblack

bolasblack Dec 3, 2013

Contributor

It's a batter solution.

Contributor

jonathanong commented Dec 3, 2013

also, some sort of docs would be good, even if it's just a line of works with nested functions, too!

Contributor

bolasblack commented Dec 3, 2013

@jonathanong 👌 document added~

Contributor

jonathanong commented Dec 4, 2013

ohhh noo! i just merged your other PR now i can't merge this one. i think everything looks good now. if anyone wants to add any input before i merge this...

Contributor

bolasblack commented Dec 4, 2013

@jonathanong I will rebase this branch.

Contributor

bolasblack commented Dec 4, 2013

@jonathanong All things done now 😆

@tj tj commented on an outdated diff Dec 4, 2013

+ subtract: makeArgsInt(function(a, b) { return a - b }),
+ multiply: makeArgsInt(function(a, b) { return a * b }),
+ divide: makeArgsInt(function(a, b) { return a / b }),
+ floor: makeArgsInt(Math.floor)
+}
+
+rework(fixture('function.nested'))
+ .use(rework.function(functions))
+ .should.equal(fixture('function.nested.out'));
+
+function makeArgsInt(cb) {
+ return function(a, b) {
+ a = parseInt(a, 10);
+ b = parseInt(b, 10);
+ return cb(a, b);
+ }
@tj

tj Dec 4, 2013

Owner

I'd ditch this function since it complicates the example a bit, math on the strings should still work

@tj tj commented on an outdated diff Dec 4, 2013

lib/plugins/function.js
declarations.forEach(function(decl){
if ('comment' == decl.type) return;
- if (!~decl.value.indexOf(name + '(')) return;
- decl.value = decl.value.replace(regexp, function(_, args){
- if (!parseArgs) return func.call(decl, strip(args));
- args = args.split(/,\s*/).map(strip);
- return func.apply(decl, args);
- });
+ var generatedFuncs = [];
+
+ while (decl.value.match(functionMatcher)) {
+ decl.value = decl.value.replace(functionMatcher, function(_, name, args){
+ // Ensure result is string
+ var result = '';
+
+ if (!parseArgs) {
@tj

tj Dec 4, 2013

Owner

negative with an else below could just be switched

Contributor

bolasblack commented Dec 4, 2013

@visionmedia I think it's ok now 👌

@jonathanong jonathanong commented on an outdated diff Dec 5, 2013

lib/plugins/function.js
- decl.value = decl.value.replace(regexp, function(_, args){
- if (!parseArgs) return func.call(decl, strip(args));
- args = args.split(/,\s*/).map(strip);
- return func.apply(decl, args);
- });
+ var generatedFuncs = [];
+
+ while (decl.value.match(functionMatcher)) {
+ decl.value = decl.value.replace(functionMatcher, function(_, name, args){
+ if (parseArgs) {
+ args = args.split(/\s*,\s*/).map(strip);
+ } else {
+ args = [strip(args)];
+ }
+ // Ensure result is string
+ var result = '' + functions[name].apply(decl, args);
@jonathanong

jonathanong Dec 5, 2013

Contributor

much cleaner :D

@jonathanong jonathanong commented on the diff Dec 5, 2013

lib/plugins/function.js
+ args = args.split(/\s*,\s*/).map(strip);
+ } else {
+ args = [strip(args)];
+ }
+ // Ensure result is string
+ var result = '' + functions[name].apply(decl, args);
+
+ // Prevent fall into infinite loop like this:
+ //
+ // {
+ // url: function(path) {
+ // return 'url(' + '/some/prefix' + path + ')'
+ // }
+ // }
+ //
+ generatedFunc = {from: name, to: name + getRandomString()};
@jonathanong

jonathanong Dec 5, 2013

Contributor

where is the var for this? i think we have a leak.

@bolasblack

bolasblack Dec 5, 2013

Contributor

Ohhhhh....I forgot declare generatedFunc...

@jonathanong jonathanong commented on an outdated diff Dec 5, 2013

+
+```css
+input {
+ top: divide(subtract(30, floor(multiply(20, 10))), 2);
+}
+```
+
+```javascript
+var functions = {
+ subtract: function(a, b) { return a - b },
+ multiply: function(a, b) { return a * b },
+ divide: function(a, b) { return a / b },
+ floor: makeArgsInt(Math.floor)
+}
+
+rework(fixture('function.nested'))
@jonathanong

jonathanong Dec 5, 2013

Contributor

haha the test is unnecessary here. just .use(rework.function(functions)) should be fine. you can do it all in one code block: .use(rework.function({subtract: ...}))

Contributor

bolasblack commented Dec 5, 2013

@jonathanong How about now~~

Contributor

jonathanong commented Dec 5, 2013

yup! thank you!

jonathanong merged commit 5d7dc5d into reworkcss:master Dec 5, 2013

1 check passed

default The Travis CI build passed
Details

bolasblack deleted the bolasblack:support-nested-function branch Dec 5, 2013

Contributor

bolasblack commented Dec 5, 2013

@jonathanong @visionmedia Thanks for all your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment