New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix argument concatenation with $
#553
Conversation
); | ||
}; | ||
|
||
export const parseTemplates = (templates, expressions) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use Array.reduce()
too, but it is forbidden by xo, so I opted for a for
loop instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch @ehmicky! Totally agree this is the behavior I would expect 👍
test('$ can concatenate at the start of tokens followed by an array', async t => { | ||
const {stdout} = await $`echo.js ${['foo', 'bar']}foo`; | ||
t.is(stdout, 'foo\nbarfoo'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, but this might be another good one to include:
test('$ can concatenate at the start and end of tokens surrounding an array', async t => {
const {stdout} = await $`echo.js foo${['bar', 'baz']}foo`;
t.is(stdout, 'foobar\nbazfoo');
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! Added in 366e6fc
f5ce1df
to
366e6fc
Compare
Looks like this bug fix was quite necessary, we are getting some bug report about it: #554 |
@@ -76,21 +76,45 @@ const parseExpression = expression => { | |||
throw new TypeError(`Unexpected "${typeOfExpression}" in template expression`); | |||
}; | |||
|
|||
const parseTemplate = (template, index, templates, expressions) => { | |||
const concatTokens = (tokens, nextTokens, isNew) => isNew || tokens.length === 0 || nextTokens.length === 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I still prefer the immutable composition and readability of your parseTempates
/concatTokens
update @ehmicky, but was curious what it would look like to push and pop from a single tokens
array:
const SPACES_REGEXP = / +/g
/** @typedef {string | number | (string | number)[]} TemplateExpression */
/**
* @type {(templates: TemplateStringsArray, ...expressions: TemplateExpression[]) => void}
*/
function $(templates, ...expressions) {
const [file, ...args] = parseTemplates(templates, expressions)
console.log('file:', file)
console.log('args:', args)
}
$`foo bar${'baz'} foo${['bar', 'baz']}foo`
//=> file: foo
//=> args: [barbaz, foobar, bazfoo]
/**
* @type {(templates: TemplateStringsArray, expressions: TemplateExpression[]) => string[]}
*/
function parseTemplates(templates, expressions) {
/** @type {string[]} */
const tokens = []
for (const [i, template] of templates.entries()) {
const templateString = template ?? templates.raw[i]
const templateTokens = templateString.split(SPACES_REGEXP).filter(Boolean)
if (
templateString.startsWith(' ') ||
tokens.length === 0 ||
templateTokens.length === 0
) {
tokens.push(...templateTokens)
} else {
for (const [j, templateToken] of templateTokens.entries()) {
tokens.push(j === 0 ? `${tokens.pop()}${templateToken}` : templateToken)
}
}
if (i === expressions.length) break
const expression = expressions[i]
const expressionTokens = Array.isArray(expression)
? expression.map((expression) => parseExpression(expression))
: [parseExpression(expression)]
if (
templateString.endsWith(' ') ||
tokens.length === 0 ||
expressionTokens.length === 0
) {
tokens.push(...expressionTokens)
} else {
for (const [j, expressionToken] of expressionTokens.entries()) {
tokens.push(
j === 0 ? `${tokens.pop()}${expressionToken}` : expressionToken,
)
}
}
}
return tokens
}
/** @type {(expression: string | number) => string} */
function parseExpression(expression) {
if (typeof expression === 'string') {
return expression
}
if (typeof expression === 'number') {
return String(expression)
}
throw new TypeError(
`Unexpected "${typeof expression}" in template expression`,
)
}
Im guessing the perf difference would be negligible, but thought I’d share the exploration!
The
$
method concatenates arguments incorrectly currently. For example:Currently behaves the same way as:
I.e. it evaluates to
mkdir /foo /bar
. Instead, it should result inmkdir /foo/bar
. Concatenation is a very common use case, especially (but not limited to) file paths.This PR fixes this problem.