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.

Sign up for GitHub

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

Jump to bottom

Fix argument concatenation with $ #553

Merged
merged 3 commits into from Mar 14, 2023
Merged

Fix argument concatenation with $ #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8f5f35
Fix argument concatenation with `$`
ehmicky Mar 12, 2023
e85cc95
Refactoring
ehmicky Mar 12, 2023
366e6fc
Add a test
ehmicky Mar 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions docs/scripts.md
View file
Open in desktop
Expand Up @@ -140,6 +140,26 @@ const example = await $`echo example`;
await $`echo ${example}`;
```

### Concatenation

```sh
# Bash
tmpDir="/tmp"
mkdir "$tmpDir/filename"
```

```js
// zx
const tmpDir = '/tmp'
await $`mkdir ${tmpDir}/filename`;
```

```js
// Execa
const tmpDir = '/tmp'
await $`mkdir ${tmpDir}/filename`;
```

### Parallel commands

```sh
Expand Down
40 changes: 32 additions & 8 deletions lib/command.js
View file
Open in desktop
Expand Up @@ -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!

? [...tokens, ...nextTokens]
: [
...tokens.slice(0, -1),
`${tokens[tokens.length - 1]}${nextTokens[0]}`,
...nextTokens.slice(1),
];

const parseTemplate = ({templates, expressions, tokens, index, template}) => {
const templateString = template ?? templates.raw[index];
const templateTokens = templateString.split(SPACES_REGEXP).filter(Boolean);
const newTokens = concatTokens(
tokens,
templateTokens,
templateString.startsWith(' '),
);

if (index === expressions.length) {
return templateTokens;
return newTokens;
}

const expression = expressions[index];
const expressionTokens = Array.isArray(expression)
? expression.map(expression => parseExpression(expression))
: [parseExpression(expression)];
return concatTokens(
newTokens,
expressionTokens,
templateString.endsWith(' '),
);
};

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.

let tokens = [];

return Array.isArray(expression)
? [...templateTokens, ...expression.map(expression => parseExpression(expression))]
: [...templateTokens, parseExpression(expression)];
for (const [index, template] of templates.entries()) {
tokens = parseTemplate({templates, expressions, tokens, index, template});
}

return tokens;
};

export const parseTemplates = (templates, expressions) => templates.flatMap(
(template, index) => parseTemplate(template, index, templates, expressions),
);
45 changes: 45 additions & 0 deletions test/command.js
View file
Open in desktop
Expand Up @@ -112,6 +112,11 @@ test('$ allows array interpolation', async t => {
t.is(stdout, 'foo\nbar');
});

test('$ allows empty array interpolation', async t => {
const {stdout} = await $`echo.js foo ${[]} bar`;
t.is(stdout, 'foo\nbar');
});

test('$ allows execa return value interpolation', async t => {
const foo = await $`echo.js foo`;
const {stdout} = await $`echo.js ${foo} bar`;
Expand Down Expand Up @@ -171,6 +176,46 @@ test('$ handles invalid escape sequence', async t => {
t.is(stdout, '\\u');
});

test('$ can concatenate at the end of tokens', async t => {
const {stdout} = await $`echo.js foo${'bar'}`;
t.is(stdout, 'foobar');
});

test('$ does not concatenate at the end of tokens with a space', async t => {
const {stdout} = await $`echo.js foo ${'bar'}`;
t.is(stdout, 'foo\nbar');
});

test('$ can concatenate at the end of tokens followed by an array', async t => {
const {stdout} = await $`echo.js foo${['bar', 'foo']}`;
t.is(stdout, 'foobar\nfoo');
});

test('$ can concatenate at the start of tokens', async t => {
const {stdout} = await $`echo.js ${'foo'}bar`;
t.is(stdout, 'foobar');
});

test('$ does not concatenate at the start of tokens with a space', async t => {
const {stdout} = await $`echo.js ${'foo'} bar`;
t.is(stdout, 'foo\nbar');
});

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


test('$ can concatenate at the start and end of tokens followed by an array', async t => {
const {stdout} = await $`echo.js foo${['bar', 'foo']}bar`;
t.is(stdout, 'foobar\nfoobar');
});

test('$ can concatenate multiple tokens', async t => {
const {stdout} = await $`echo.js ${'foo'}bar${'foo'}`;
t.is(stdout, 'foobarfoo');
});

test('$ allows escaping spaces in commands with interpolation', async t => {
const {stdout} = await $`${'command with space.js'} foo bar`;
t.is(stdout, 'foo\nbar');
Expand Down