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

[commonjs] strictRequires indent with \t modifies multi-line string values. #1228

Closed
simonbuchan opened this issue Aug 1, 2022 · 2 comments · Fixed by #1229
Closed

[commonjs] strictRequires indent with \t modifies multi-line string values. #1228

simonbuchan opened this issue Aug 1, 2022 · 2 comments · Fixed by #1229

Comments

@simonbuchan
Copy link
Contributor

Expected Behavior

Multiline-string values should be preserved.

Actual Behavior

Indent \t characters are blindly inserted into the multi-line string.

Additional Information

This particular issue seems to be due to:

magicString.trim().indent('\t');

but there are other uses of magicString.indent("\t") in the code-base, e.g.:

magicString
.trim()
.indent('\t')
.prepend(`(function (${args.join(', ')}) {\n`)
.append(`\n} (${passedArgs.join(', ')}));`);

The simple fix is just don't indent code - I doubt it would be simple to implement a correct source-map-preserving indent. Perhaps instead doing a transform from multi-line template strings to single-line escape codes would also work?

@lukastaegert
Copy link
Member

lukastaegert commented Aug 1, 2022

Actually considering that the plugin is already parsing the entire code and knows all template literals, the fix is to pass an indentExclusionRanges array to the MagicString constructor and for every TemplateLiteral, push [start, end] into the array. This should actually be done for regular Literals as well as you can escape a line-break in a string. This is how Rollup does it https://github.com/rollup/rollup/blob/release-3.0.0/src/ast/nodes/TemplateLiteral.ts#L57-L57 https://github.com/rollup/rollup/blob/release-3.0.0/src/ast/nodes/Literal.ts#L88-L88, see also https://github.com/Rich-Harris/magic-string#usage

@simonbuchan
Copy link
Contributor Author

As a workaround for now, I'm flattening multi-line strings with:

import * as babelTypes from "@babel/types"

import babel from "@rollup/plugin-babel";
import commonjs from "@rollup/plugin-commonjs";

export default {
  ...
  plugins: [
    ...
    babel({
      skipPreflightCheck: true,
      babelHelpers: "external",
      babelrc: false,
      configFile: false,
      plugins: [babelPluginFlattenTemplate()],
    }),
    commonjs(),
    ...
  ],
};

function babelPluginFlattenTemplate(): babel.PluginObj<unknown> {
  return {
    visitor: {
      TemplateElement(path) {
        const value = path.node.value;
        if (value.raw.includes("\n")) {
          path.replaceWith(
            babelTypes.templateElement({
              raw: value.raw.replace(/\n/g, "\\n"),
              cooked: value.cooked?.replace(/\n/g, "\\n"),
            }),
          );
        }
      },
    },
  };
}

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 a pull request may close this issue.

2 participants