fix(build): Fix regex in multiline-comment-removal Rollup plugin

[lobsterkatie]

The existing regex, when presented with code like this:

/* 
 * This is
 * a multiline
 * comment 
 */

const thisIs = "real code"

/* 
 * This is
 * another multiline
 * comment 
 */

const thisIsAlso = "real code"

matches everything between the initial /* on the first line and the final */ on the second-to-last line, and the plugin thus removes all but the last line of the file. This fixes the regex so that only the comments are removed.

[lforst]

Why are we only removing multiline comments?

If we were to remove all comments we could simply use a library to do this for us (eg terser), and avoid reinventing the wheel and running into issues that have already been solved.

Something like the following maybe?

terser({
  // remove all comments
  format: {
    comments: false
  },
  // prevent any compression
  compress: false
})

source

[lobsterkatie]

Why are we only removing multiline comments?

If we were to remove all comments we could simply use a library to do this for us (eg terser), and avoid reinventing the wheel and running into issues that have already been solved.

Both reasonable questions. Mostly what you're seeing is that I originally just wanted a super-fast hack to clean up the generated templates, and it didn't feel worth it to involve as much machinery as terser (or even a dedicated plugin like https://github.com/aMarCruz/rollup-plugin-cleanup) provides. (Also, for things like getting rid of eslint comments, I actually couldn't find any way other than regex to do it, so I think that’s where my brain was.)

As for why only multi-line comments... that just happens to be the ones I wanted to get rid of. 🙂 And in the particular case of the templates, I do think there's some small utility in keeping the one // comment which remains, though I don't feel super strongly about it.

So... keep the hack? Don't keep the hack (which means getting rid of all comments)? As much as it is admittedly a tiny bit uglier, I lean ever-so-slightly towards keeping the hack, and perhaps changing the note to label it as such and point to a library rather than give the fuller regex. It would mean that the one useful comment can stay, and it would also save the time of debugging a new plugin. Open to being swayed on that score, though.

UPDATE: I was torn, so I checked it out. Terser is definitely more than what we want (with default settings on, it makes all sorts of other changes to the code I'm not interested in), and cleanup({ comments: 'none' }) (from the plugin I linked) seemingly has no effect. Rather than spend time debugging either, I'm going to leave the (fixed) regex in place, but I'll add a note in case we want to come back to this in the future.

[lforst]

Writing from my phone so no fancy formatting: Can you try adding the "extensions" option with "ts" to the cleanup plugin? It would calm my nerves knowing that a dedicated library is taking care of this. Sorry if this is annoying.

[lforst]

I also quickly discussed this with @AbhiPrasad and we are both of the opinion that attempting to remove comments with a regex is a bit dangerous and that the upside of only having single-line comments is not entirely clear.

In order to reduce review/discussion cycles I decided to open a PR that implements rollup-plugin-cleanup to remove all of the comments in our generated output - whatever our path forward may be: #5799

[lobsterkatie]

I also quickly discussed this with @AbhiPrasad and we are both of the opinion that attempting to remove comments with a regex is a bit dangerous and that the upside of only having single-line comments is not entirely clear.

In order to reduce review/discussion cycles I decided to open a PR that implements rollup-plugin-cleanup to remove all of the comments in our generated output - whatever our path forward may be: #5799

Since this only affects three files, and we're still discussing the best long-term solution here, I'd like to merge this so that I can unblock other PRs. Can we do that and continue the conversation offline, please?

[lforst]

I'd like to merge this so that I can unblock other PRs. Can we do that and continue the conversation offline, please?

Sounds good!