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

istanbul comments stripped when compiling in jest-transform #516

Closed
LeeCheneler opened this issue Nov 10, 2020 · 14 comments
Closed

istanbul comments stripped when compiling in jest-transform #516

LeeCheneler opened this issue Nov 10, 2020 · 14 comments

Comments

@LeeCheneler
Copy link

I have a jest-transform I'm using on a project (v2 beta of https://github.com/ts-engine/ts-engine) that compiles with esbuild and it works great, istanbul coverage works fine too. However because comments are stripped from the compiled output you can't ignore statements/files etc on the occasions you need to. An option to maintain comments would be great if possible.

Here is the jest transform for reference:

"use strict";

const path = require("path");
const fs = require("fs-extra");
const esbuild = require("esbuild");
const uuid = require("uuid");

// All temp files are stored in the same place with a cache busting name
const getTempFilename = (filename, cacheBuster) => {
  return path.join(
    require.resolve("./package.json").replace("package.json", ""),
    "/node_modules/.temp/jest-transformer",
    `${cacheBuster}-${filename}`
  );
};

module.exports = {
  getCacheKey: () => {
    // Forces Jest to ignore cache
    return Math.random().toString();
  },
  process: (src, filename) => {
    const cacheBuster = uuid.v4();
    const filenameParts = filename.split("/");
    const file = filenameParts[filenameParts.length - 1];

    const jsFilename = getTempFilename(`${file}`, cacheBuster);
    const sourcemapFilename = getTempFilename(`${file}.map`, cacheBuster);

    // build file using esbuild, don't perform bundling or it will break code coverage
    esbuild.buildSync({
      sourcemap: "external",
      format: "cjs",
      entryPoints: [filename],
      target: "node10.13",
      platform: "node",
      outfile: jsFilename,
    });

    // read compiled js and sourcemap, delete the temp files as they aren't needed again
    let js = fs.readFileSync(jsFilename, "utf-8");
    fs.removeSync(jsFilename);
    let sourcemap = fs.readFileSync(sourcemapFilename, "utf-8");
    fs.removeSync(sourcemapFilename);

    return {
      code: js,
      map: JSON.parse(sourcemap),
    };
  },
};

Adding an option like preserveComments: true to esbuild would fix this issue.

@evanw
Copy link
Owner

evanw commented Nov 11, 2020

You are correct, esbuild does not preserve comments. It's not built to preserve comments. Preserving comments means it would operate more like a code formatter than a compiler. For example, you would probably want a token spanning tree instead of an AST. Doing this is currently a non-goal of esbuild. You should probably be using another tool for this.

@LeeCheneler
Copy link
Author

LeeCheneler commented Nov 11, 2020

This is unfortunate... Guess esbuild is a none starter for JS/TS tooling then 😔 will check out swc and see how it fairs.

Super job overall, worked really well until this wall.

@matrunchyk
Copy link

matrunchyk commented May 3, 2022

Wow, that's a bummer. Doesn't AST support something like node.leadingComments, which babel does?

@Hideman85
Copy link

I'm seing that there is a support of legalComments: inline https://esbuild.github.io/api/#legal-comments
I'm wondering if this is possible to include manually some comments?

I just switched from babel or ts in my jest testing using esbuild and this is blazing fast that is pretty awesome to do TDD in watch mode. But then if we cannot instrument our code to tell jest via comments to ignore some parts we cannot match our requirements and that is very frustrating.

Esbuild start to have a great visibility because it is blazing fast, there lot of projects on top of it like Vite for the FE but then there is a few limitations that you cannot use it as a drop replacement of the well known tools and I think that's a shame.

Maybe I have to look for swc too...

@CSchulz
Copy link

CSchulz commented Aug 12, 2022

Can't you use //! or /*! for istanbul coverage ignore?

@Hideman85
Copy link

Can't you use //! or /*! for istanbul coverage ignore?

Does not seems to work neither //! istanbul ignore next nor /*! istanbul ignore next */

@LeeCheneler
Copy link
Author

yeah most tooling ignores that format of comments so its pretty useless tbh unfortunately

@xnuk
Copy link

xnuk commented Aug 15, 2022

In case of Greasemonkey, the user script tool, file must start with // ==UserScript== exactly, follows by metadata block, and // ==/UserScript==: https://github.com/greasemonkey/greasemonkey/blob/11e340b4dadee868191e72a3ef888f75c6eabbc1/src/parse-user-script.js#L3

// ==UserScript==
// @name Foobar
// @version 1
// ==/UserScript==

window.alert('1')

which is esbuild just removes it, and makes the user script does not work.

@evanw
Copy link
Owner

evanw commented Aug 15, 2022

file must start with // ==UserScript== exactly

You're looking for this feature: https://esbuild.github.io/api/#banner

@xnuk
Copy link

xnuk commented Aug 15, 2022

@evanw

file must start with // ==UserScript== exactly

You're looking for this feature: https://esbuild.github.io/api/#banner

How?

const { code } = require('esbuild').transformSync(
	[
		'// ==UserScript==',
		'// @name Foobar',
		'// @version 1',
		'// ==/UserScript==',
		'',
		'window.alert("1")',
	].join('\n'),
	{
		banner: '// ==UserScript==', // <<---- how?
	},
)

if (!code.includes('@version')) throw new Error('Metadata not preserved')

@Hideman85
Copy link

@xnuk
He meant the following:

const banner = `// ==UserScript==
// @name Foobar
// @version 1
// ==/UserScript==
`
const source = `
window.alert("1")
`
const { code } = require('esbuild').transformSync(source, { banner })

if (!code.includes('@version')) throw new Error('Metadata not preserved')

But to be honest, in your example the easiest workaround would be directly this:

const banner = `// ==UserScript==
// @name Foobar
// @version 1
// ==/UserScript==

`
const source = `
window.alert("1")
`
const { code } = require('esbuild').transformSync(source)
fs.writeFileSync('index.js', banner + code)

My original topic about istanbul ignore is that the position in the code is important. To be honest there is exception for legal comments that need to match a specific regex but we cannot have these comments, and I dont feel there is a willing to change this.

Nevermind it seems that everyone fallback to use swc that gives a performance boost from babel/webpack and is not that strict on rules like these.
For me the one strict rule is People > Processes > Tools, if the tool does not satisfy your needs just change it.

@LeeCheneler
Copy link
Author

SWC provides an option to preserve comments in its compiler options. This is perfect because there are instances when you want to strip comments and when you want to preserve them. A transform for jest is perfect example of when you want to preserve comments for tooling like istanbul, stripping the comments in a jest transform instantly makes it inviable unfortunately.

We moved to SWC and have found it much easier to work with and allows much better compatibility with tooling. Still hoping esbuild becomes more tooling friendly in future.

@AriPerkkio
Copy link

vitest is also facing the same issue it seems, vitest-dev/vitest#2021.

Can't you use //! or /*! for istanbul coverage ignore?

Does not seems to work neither //! istanbul ignore next nor /*! istanbul ignore next */

As work-around you can use /* istanbul ignore -- @preserve */ with "legalComments": "inline" option. This gets identified by istanbul properly. But note that these will end up in production build as well. 😞

https://esbuild.egoist.dev/#W1siaW5kZXgudHMiLHsiY29udGVudCI6ImV4cG9ydCBmdW5jdGlvbiBtZXRob2QoYXJnKSB7XG4gICAvLyBUaGlzIGlzIG5vdCBwcmVzZXJ2ZWRcbiAgIC8qIGlzdGFuYnVsIGlnbm9yZSBpZiAqL1xuICAgIGlmKGFyZyA9PT0gMikge1xuICAgIHJldHVybiAxO1xuICAgIH1cbiAgICBcbiAgIC8vIFRoaXMgaXMgcHJlc2VydmVkICAgICBcbiAgIC8qIGlzdGFuYnVsIGlnbm9yZSBpZiAtLSBAcHJlc2VydmUgKi9cbiAgIGlmKGFyZyA9PT0gMykge1xuICAgIHJldHVybiAzO1xuICAgIH1cbn0ifV0sWyJlc2J1aWxkLmNvbmZpZy5qc29uIix7ImNvbnRlbnQiOiJ7XG4gIFwiZm9ybWF0XCI6IFwiY2pzXCIsXG4gIFwiY2RuVXJsXCI6IFwiaHR0cHM6Ly9jZG4uc2t5cGFjay5kZXZcIixcbiAgXCJsZWdhbENvbW1lbnRzXCI6IFwiaW5saW5lXCJcbn0ifV0sWyJzdW0udHMiLHsiY29udGVudCI6ImV4cG9ydCBjb25zdCBzdW0gPSAoYTogbnVtYmVyLCBiOiBudW1iZXIpID0+IGEgKyBiIn1dXQ==

@raveclassic
Copy link

raveclassic commented Apr 11, 2024

@evanw Sorry for bumping this, what would would be the idiomatic way for you to use esbuild with istanbul ignore comments? Turns out this is a fundamental feature and its absence sort of breaks vitest coverage.

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

No branches or pull requests

8 participants