fix(strip): support specifying both functions and labels #471

dgkimpton · 2020-06-23T13:51:51Z

Rollup Plugin Name: `strip`

This PR contains:

Are tests included?

yes
no

Breaking Changes?

yes
no

List any relevant issue numbers:
#167

Description

When trying to use the strip plugin, I was for ages unable to get it to remove my labelled code. After some investigation, it became clear that the 'first pass filter' was rejecting labels when functions were specified.

This pull request fixes the exclusive nature of labels and functions. It is now possible to specify both, either, or neither.

I later found this 'closed' issue that reports the same problem, #167, it appears to have been started and then abandoned - hope I'm not treading on toes by fixing it.

Along the way, I had some trouble with the unit tests and got annoyed that I couldn't see the input and expected output together, so I refactored them and then added some more.

During testing, I further discovered that labels and functions were being ignored when spaces/newlines were included around the punctuation, so I fixed that too. Probably that should have been a different PR, but since my use-case had spaces in it, I just kept going until it worked. Sorry about that!

config. FIX: Supports spaces around the . for functions and : for labels DOC: Updated documentation REFACTOR: Adjust tests so that the input and expected output can be seen next to each other.

Kocal

TBH I was only able to understand the tests, and they look good to me.

Thanks for your work!

pnevares

Looks like some big changes here, but the work is appreciated! The only thing I'd challenge is that there are breaking changes, if labels were passing by the filter when functions were specified. It's probably up to personal opinion, but to me that difference means that a user may be inadvertently relying on that implementation difference and this fix would break their use case.

Otherwise, LGTM.

dgkimpton · 2020-06-26T05:18:15Z

I see your point. That would be a bit of a hairy build process, but I suppose it is conceivable. The only way I can think to resolve that is to add a new option with a different name.
E.g. labels:[] has the current functionality and labeled:[] works in the none exclusive mode.
That can definitely be done but explodes the complexity quite a bit... is that your preferred option, or can you think of an alternative?

pnevares · 2020-07-05T09:00:36Z

I would probably prefer marking it as a breaking change and a major version bump for the package, instead of adding the technical debt of the newly-named option. Somebody would have an item backlogged to clean that up for the next version anyway.

shellscape

In addition to the changes requested in the source, you'll need to revert your deletions of the fixtures and modifications of the tests. We've established a pattern for testing in this plugin and contributors should follow that. Completely refactoring the tests in this plugin (or any project) is going too far without previous discussing that action in an issue.

As it is, we cannot merge this PR.

shellscape · 2020-07-06T16:26:21Z

packages/strip/README.md

-      labels: ['unittest']
-    })
-  ]
+  plugins: [strip({})]


please revert this change

shellscape · 2020-07-06T16:26:45Z

packages/strip/README.md

 };
 ```

 Then call `rollup` either via the [CLI](https://www.rollupjs.org/guide/en/#command-line-reference) or the [API](https://www.rollupjs.org/guide/en/#javascript-api).

+### Default Options


please remove this section. the defaults are spelled out in the options below

I added this because I wished it had been here when I started trying to use this plugin, but ok.

shellscape · 2020-07-06T16:27:10Z

packages/strip/README.md

+> remove functions matching '**console.\***' and '**assert.\***'  
+> remove any **debugger** statements
+
+### Custom Options Example


please remove this section. you've added examples in the options below and that's sufficient

shellscape · 2020-07-06T16:27:58Z

packages/strip/README.md


-If `true`, instructs the plugin to remove debugger statements.
+If `true` or unspecified, instructs the plugin to remove debugger statements.


or unspecified is not not necessary and doesn't add any value here, please revert.

packages/strip/src/index.js

shellscape · 2020-07-06T16:32:23Z

packages/strip/src/index.js

+  );
+  const labelsPatterns = labels.map((l) => `${l}\\s*:`);
+
+  const firstPassPatterns = removeDebuggerStatements


this ternary and the following array assignment within is unnecessary.

const firstPass = [...functionsPatterns, ...labelsPatterns]; if (removeDebuggerStatements) { firstPass.push('debugger\\b'); }

Ooops, that would be my C++ background where const is deep. I'd forgotten in JS const is shallow and still allows the object to be mutated. Done.

shellscape · 2020-07-06T16:34:27Z

packages/strip/src/index.js

+    ? [...functionsPatterns, ...labelsPatterns, 'debugger\\b']
+    : [...functionsPatterns, ...labelsPatterns];
+
+  const functionsRE = new RegExp(`^(?:${functionsPatterns.join('|')})$`);


while we're in here, please use the naming pattern reFunctions and reFirstpass

dgkimpton · 2020-07-08T14:07:41Z

OK, I figured that would probably be the case. I modified the tests to make it easier for me to verify the fix and then decided just to offer them up as-is in case you preferred them (no point doing extra work if not needed). Changed.

shellscape · 2020-07-08T16:44:08Z

Thanks. Please clean up those file mode changes so they're not part of the PR and we should be good to merge.

packages/strip/README.md

packages/strip/src/index.js

shellscape · 2020-07-09T13:41:56Z

* FIX: Now supports specifying both functions and labels in the same config. FIX: Supports spaces around the . for functions and : for labels DOC: Updated documentation REFACTOR: Adjust tests so that the input and expected output can be seen next to each other. * restoring test fixtures * code review changes * additional tests * attempt to clean up filemodes * revert subjective style change * revert extra newline * revert extra newlines Co-authored-by: Andrew Powell <shellscape@users.noreply.github.com>

dasa · 2021-05-06T21:50:44Z

It looks like there was a publishing problem and this PR wasn't included in the 2.0.0 release: https://unpkg.com/@rollup/plugin-strip@2.0.0/dist/index.es.js

For example, the published file is still using if (!edited) return null;.

dasa · 2021-05-07T17:03:18Z

Fixed in 2.0.1: https://unpkg.com/@rollup/plugin-strip@2.0.1/dist/index.es.js

👍🏻

FIX: Now supports specifying both functions and labels in the same

0b91de0

config. FIX: Supports spaces around the . for functions and : for labels DOC: Updated documentation REFACTOR: Adjust tests so that the input and expected output can be seen next to each other.

shellscape changed the title ~~FIX: Now supports specifying both functions and labels at same time~~ fix(strip): support specifying both functions and labels Jun 23, 2020

shellscape added pr⁴ 🔧 fix 🔌 plugin-strip labels Jun 23, 2020

shellscape requested review from Andarist, Kocal, LarsDenBakker, pnevares, TrySound and wuweiweiwu June 23, 2020 14:49

Kocal approved these changes Jun 23, 2020

View reviewed changes

pnevares approved these changes Jun 25, 2020

View reviewed changes

shellscape requested changes Jul 6, 2020

View reviewed changes

dgkimpton added 3 commits July 8, 2020 15:19

restoring test fixtures

37df374

code review changes

06871ec

additional tests

ab96d8b

attempt to clean up filemodes

8dfaa84

shellscape reviewed Jul 9, 2020

View reviewed changes

packages/strip/README.md Outdated Show resolved Hide resolved

revert subjective style change

30ae82f

shellscape reviewed Jul 9, 2020

View reviewed changes

packages/strip/src/index.js Outdated Show resolved Hide resolved

revert extra newline

eec0465

shellscape reviewed Jul 9, 2020

View reviewed changes

packages/strip/src/index.js Outdated Show resolved Hide resolved

revert extra newlines

4524da9

shellscape approved these changes Jul 9, 2020

View reviewed changes

shellscape merged commit a014c66 into rollup:master Jul 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(strip): support specifying both functions and labels #471

fix(strip): support specifying both functions and labels #471

dgkimpton commented Jun 23, 2020 •

edited by shellscape

Kocal left a comment •

edited

pnevares left a comment

dgkimpton commented Jun 26, 2020

pnevares commented Jul 5, 2020

shellscape left a comment

shellscape Jul 6, 2020

dgkimpton Jul 8, 2020

shellscape Jul 6, 2020

dgkimpton Jul 8, 2020

shellscape Jul 6, 2020

dgkimpton Jul 8, 2020

shellscape Jul 6, 2020

dgkimpton Jul 8, 2020

shellscape Jul 6, 2020

dgkimpton Jul 8, 2020

shellscape Jul 6, 2020

dgkimpton Jul 8, 2020

dgkimpton commented Jul 8, 2020

shellscape commented Jul 8, 2020

shellscape commented Jul 9, 2020

dasa commented May 6, 2021

dasa commented May 7, 2021


		If `true`, instructs the plugin to remove debugger statements.
		If `true` or unspecified, instructs the plugin to remove debugger statements.

fix(strip): support specifying both functions and labels #471

fix(strip): support specifying both functions and labels #471

Conversation

dgkimpton commented Jun 23, 2020 • edited by shellscape

Rollup Plugin Name: strip

Description

Kocal left a comment • edited

Choose a reason for hiding this comment

pnevares left a comment

Choose a reason for hiding this comment

dgkimpton commented Jun 26, 2020

pnevares commented Jul 5, 2020

shellscape left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgkimpton commented Jul 8, 2020

shellscape commented Jul 8, 2020

shellscape commented Jul 9, 2020

dasa commented May 6, 2021

dasa commented May 7, 2021

dgkimpton commented Jun 23, 2020 •

edited by shellscape

Rollup Plugin Name: `strip`

Kocal left a comment •

edited