Support functions for all options (fix #139) #168
Conversation
|
@hgwood thanks for submitting this. I believe we wanted the logic to be applied to
|
Will do.
I meant to forward the glob to function-options but forgot. Is the glob what you mean by "vinyl file object"? Otherwise could you specify what you mean? I don't see anything in
Just to be sure we share the same understanding:
If vinyl-fs supported passing function-options for through2 options then how could it distinguish between someone trying to pass a function to through2 and someone trying to pass a function-option that returns a function for through2? I think this could become very confusing for users. So I was careful to stay clear of this and my implementation only calls function-options for vinyl-fs own options.
Well, I'm not a fan of this (I prefer to embrace coercion), but I have noticed that's how vinyl-fs currently works: all options are type-checked already. As a result, adding Quick pros and cons of
That said, this is pretty much up to personal preference. You are the maintainer of this project and you decide. :) |
|
@phated Any reason why we shouldn't apply this across the board? (src, dest, and symlink) |
|
I've applied the behavior to |
|
I have noticed |
|
@hgwood correct, |
|
The new proposal leverages I guess the only significant remaining question is: do we agree on the fact that we should only process options that are for vinyl-fs itself? See the second point in my earlier comment. |
|
@hgwood I'm not really happy with the colliding options, so maybe our |
|
@phated Sorry I think I might have confused you. The colliding option is actually not the problem. You said:
The thing is, some through2 options are functions (namely |
|
@hgwood The Edit: I agree that we shouldn't process options that vinyl-fs doesn't know or care about. Again, not the issue at hand in regards to |
|
Oh, right. The |
|
Renaming the |
|
Separate namespaces get really messy, as we pass options down through glob-stream into node-glob and so forth. Would they be objects nested in objects nested in objects? How far does it go? It seems really clunky |
|
True, it's not pretty. But eliminates collisions, current or future. Renaming the vinyl-fs option (to what?) is also not exactly elegant, imho. I don't have a strong preference either way, but since you asked :-) |
| var overwrite = booleanOrFunc(options.overwrite, file); | ||
| options.flag = (overwrite ? 'w' : 'wx'); | ||
| var options = normalizeOptions(opt, { | ||
| cwd: { type: 'string', defaultValue: process.cwd() }, |
There was a problem hiding this comment.
This is awkward. Would there be a better way to implement it? Maybe something like defaultValue(valueOrFunction.string(opt.cwd, file), process.cwd())?
There was a problem hiding this comment.
Could you explain why you find it awkward?
As for the alternative solution, do you mean I should duplicate this line for every option? Like:
var options = {
cwd: defaultValue(valueOrFunction.string(opt.cwd, file), process.cwd()),
mode: defaultValue(valueOrFunction.number(opt.mode, file), (file.stat ? file.stat.mode : null)),
dirMode: ...
};
There was a problem hiding this comment.
I feel it's awkward because you invented a DSL for the option + type + default. Your alternative is kind of what I was thinking. It might even look cleaner if we "destructured" the methods from value-or-function, e.g. var str = require('value-or-function').string;
There was a problem hiding this comment.
I understand what you're saying. I'll have another go.
|
@phated I've refactored the code. Tell me if you think it's better. |
| } | ||
|
|
||
| var options = assign({}, opt, { | ||
| buffer: normalizeOption('boolean', true, opt.buffer, glob), |
There was a problem hiding this comment.
not sure how I'm feeling about the glob being passed in now that I see it. I don't think it is useful to determine the options. I'm going to think a bit more on a way to defer option resolution until we have a Vinyl File object.
There was a problem hiding this comment.
Removed glob. Deferring to get the file can be a PR on its own I think.
|
|
||
| Default: The part of the path before the glob (if any) begins. For example, `path/to/**/*.js` would resolve to `path/to`. If there is no glob (i.e. a file path with no pattern), then the dirname of the path is used. For example, `path/to/some/file.js` would resolve to `path/to/some`. | ||
| - Values passed to the options must be of the right type, otherwise they will be ignored. | ||
| - All options can be passed a function instead of a value. The function will be called with `glob` as its only argument and must return a value of the right type for the option. |
There was a problem hiding this comment.
the functions aren't called with the glob
|
@hgwood just a few things. Feel free to squash everything together if you want. |
It is not entirely clear from the issue if it is only about
srcor ifdestis also in scope. Anyway, here it is forsrc. I can replicate the behavior fordestvery simply.A few notes:
readoption fromsrctothrough2? #153) and can complexify the implementation (see how I need to avoid calling through2 options that are functions?). May I suggest moving through2 options to a through2 property in vinyl-fs options? I believe that would be simpler and clearer, both for the caller and the callee. It would be a breaking change.value-or-functionbecause I did not see the benefit of checking the types of the options. Also I saw defaulting tonullas a bit dangerous and surprising for users of vinyl-fs.