Add exclude option for transform-remove-console plugin#661
Conversation
j-f1
left a comment
j-f1
left a comment
There was a problem hiding this comment.
Great job! Just a couple of things.
Can you update the README.md for this package to explain how to provide the options?
| ); | ||
| }); | ||
|
|
||
| describe("remove-console-plugin with excludes argument", () => { |
There was a problem hiding this comment.
These tests should be updated to use the thePlugin function if possible.
There was a problem hiding this comment.
Ah, I was following the model from babel-plugin-minify-type-constructors which did not when there were options, but digging into the function prototype looks like I can pass options into thePlugin as well, will update.
|
Updated based on @j-f1's feedback |
| ```json | ||
| // with options | ||
| { | ||
| "plugins": ["transform-remove-console": { "exclude": [ "error", "warn"] }] |
There was a problem hiding this comment.
, instead of : and inside another array.
| } | ||
|
|
||
| function isConsole(memberExpr) { | ||
| function isExcluded(property, state) { |
There was a problem hiding this comment.
don't pass in the whole state. Just pass in whatever is required by this function. This function shouldn't worry about the options being available at state.opts.exclude. Just pass in the exclude array.
| ```json | ||
| // with options | ||
| { | ||
| "plugins": ["transform-remove-console", { "exclude": [ "error", "warn"] }] |
There was a problem hiding this comment.
It should be [ ["transform-remove-console", { ... } ] ]. It's an array of array of two items (name & options).
| } | ||
| }); | ||
| } | ||
| return exclude; |
There was a problem hiding this comment.
return excludeArray.some(name => property.isIdentifier({ name }));?
|
Thanks!. Looks good to me. Looks like there are some formatting errors (prettier). You can fix this by running |
| @@ -51,15 +51,7 @@ module.exports = function({ types: t }) { | |||
| } | |||
|
|
|||
| function isExcluded(property, excludeArray) { | |||
There was a problem hiding this comment.
with default param.
function isExcluded(property, excludeArray = []) {
return excludeArray.some(name => property.isIdentifier({ name }));
}There was a problem hiding this comment.
lint also failed for the same function. Please do yarn run format
|
thanks for this PR it was also something I needed Do we have to use "minify" to get this option, or does it also work with normal babel plugins? (not sure but to me it looks like it's the name plugin in both case anyway no?) |
|
babel-plugin-transform-remove-console@3.8.5 in npm did not support options? |
|
Hey, published version 6.8.5 does not include this PR |
|
If you want to use this feature right now, you can use my release (make sure to verify I'm not a malicious hacker!)
|
6 participants
An attempt to implement the extension requested in #598
Based on the unit tests I added, I believe this is working, however as this is my very first attempt at code within the babel ecosystem it is highly likely there are better ways to approach this. I welcome any and all feedback on how to improve this either functionally or idiomatically.