fix(dollar): consider \$ expression

[roggervalf]

If I do this in my terminal:

echo {a,b}\${c,d}\${e,f}
-> a$c$e a$c$f a$d$e a$d$f b$c$e b$c$f b$d$e b$d$f

now in brace-expand it do not match, so this pr is trying to fix this behavior

[isaacs]

brace-expand shouldn't be doing anything special with $ though. The \ in that example was just to keep the shell from interpreting it. If you braceExpand('${a,b}') the result should be ['$a', '$b'], imo.

[juliangruber]

Agreed! I didn't yet find the time to check why that is

[roggervalf]

hello guys, I see your point, let me also add some context about this change, so I was using minimatch package and combining it with https://www.npmjs.com/package/lodash.template, something like:

export function applyContext(str: string, context?: object) {
  if (context == null) return str
  const t = template(str)
  return t(context)
}

new Minimatch(applyContext('string ${user.id}', {user:{id:1}})).match('string 1');// true

so right now it is possible to do this, so I was also seeing the minimatch code and found that this was also covered as a functionality, to ignore the ${} expressions in https://github.com/juliangruber/brace-expansion/blob/main/test/dollar.js
Some time ago I also submitted a fix pr, for this dollar expression in #49, so I didn't add that behavior, I was thinking that the use of ${} to just ignore that expression was the expected one

[isaacs]

Yeah, I think the change in this PR is valid, if brace-expansion is going to be avoiding expansions of $-prefixed brace sections. Sorry, I didn't make that clear.

And when I look closer at the history of that feature, I see my own name scattered around the history, so... that's disconcerting and weird. Because looking at this and thinking about it really has be thinking that ${a,b} should be brace expanded to ['$a', '$b'], rather than ['${a,b}'].

Like.. wat??

> new Minimatch('$a{a,b}')
Minimatch {
  options: {},
  set: [ [ '$aa' ], [ '$ab' ] ],   // <-- totally normal
  pattern: '$a{a,b}',
  regexp: null,
  negate: false,
  comment: false,
  empty: false,
  globSet: [ '$aa', '$ab' ],
  globParts: [ [ '$aa' ], [ '$ab' ] ]
}
> new Minimatch('${a,b}')
Minimatch {
  options: {},
  set: [ [ '${a,b}' ] ],   // <-- WAT!?
  pattern: '${a,b}',
  regexp: null,
  negate: false,
  comment: false,
  empty: false,
  globSet: [ '${a,b}' ],
  globParts: [ [ '${a,b}' ] ]
}

I'm willing to bet someone somewhere is 100% depending on the current behavior, since this is used in minimatch, which is used in glob and ignore-walk, which is used in npm/cli. Which means removing the special handling of ${} is a significant breaking change, because breaking npm and glob without warning would be Bad.

So my recommendation: land this PR, as a semver patch version. Then rip out the ${} handling as a semver-major bump.

[isaacs]

I guess one potential reason that the ${} handling is there is in case someone was trying to use this to parse bash command arguments, since it doesn't do variable expansion, maybe you wouldn't want it to munge up something like echo ${foo:1:2}? But even then, I can't think of any cases where it would be a valid sequence or set, and also a valid variable expansion. But maybe I'm just not thinking hard enough, idk.

[juliangruber]

I you look at the dollar test, https://github.com/juliangruber/brace-expansion/blob/dd5a4cb21fe444c5c43f9a5d26ccba606e093107/test/dollar.js, we're relying on ${...} not getting expanded.

From https://www.gnu.org/software/bash/manual/html_node/Brace-Expansion.html:

To avoid conflicts with parameter expansion, the string ‘${’ is not considered eligible for brace expansion, and inhibits brace expansion until the closing ‘}’.

To me then the question is whether we want to stay true to bash expansion or make it potentially more useful for JS. Maybe we could also add an option for this, what do you think?

[isaacs]

From https://www.gnu.org/software/bash/manual/html_node/Brace-Expansion.html:

Ahh, ok, I didn't realize it was part of the spec. That does make more sense, then.

It's interesting that in bash at least, \${x,y} does expand, but to $x $y, not \$x \$y. If we're going to support escaping the $ to suppress the special behavior of ${, then the escaping slash needs to be dropped.

So, new tests to add:

expand('${x,y}') => ['${x,y}']  // this passes right now
expand('\\${x,y}') => ['$x', '$y']  // this fails, returns '\\${x,y}'

[juliangruber]

It's interesting that in bash at least, \${x,y} does expand, but to $x $y, not \$x \$y

I guess this kind of makes sense as \$ is an input character while \$ as an output character would be \\$ 🤔

If we're going to support escaping the $ to suppress the special behavior of ${, then the escaping slash needs to be dropped.

Are you sure we should add \\${x,y} as a test case, and not \${x,y}? Bash gives bash: \\${x,y}: bad substitution, but it might also make sense to treat characters differently here

[roggervalf]

hi @juliangruber, I have to add \\ because in strings, in order to take in count the backslash character, it needs to be added with a second backslash since it is a special character