Skip to content
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

Merged
merged 3 commits into from Mar 14, 2023
Merged

Fix argument concatenation with $ #553

merged 3 commits into from Mar 14, 2023

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Mar 13, 2023

The $ method concatenates arguments incorrectly currently. For example:

const bar = '/bar'
$`mkdir /foo${bar}`

Currently behaves the same way as:

const bar = '/bar'
$`mkdir /foo ${bar}`

I.e. it evaluates to mkdir /foo /bar. Instead, it should result in mkdir /foo/bar. Concatenation is a very common use case, especially (but not limited to) file paths.

This PR fixes this problem.

);
};

export const parseTemplates = (templates, expressions) => {
Copy link
Collaborator Author

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.

Copy link
Contributor

@aaronccasanova aaronccasanova left a 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 👍

Comment on lines +204 to +207
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');
});
Copy link
Contributor

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');
});

Copy link
Collaborator Author

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

@ehmicky ehmicky merged commit 07585d0 into main Mar 14, 2023
17 of 20 checks passed
@ehmicky ehmicky deleted the template-separation branch March 14, 2023 19:34
@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 15, 2023

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
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants