WebReflection / babel-plugin-transform-builtin-classes

A fix for the infamous Babel #4480 bug.
https://github.com/babel/babel/issues/4480
ISC License
43 stars 2 forks source link

Remove debug statement #10

Closed chocolateboy closed 6 years ago

chocolateboy commented 6 years ago
babel-plugin-transform-builtin-classes 0.4.0
nodev8.9.3, v9.2.1
OSLinux (Arch)

test.js

class Test extends Array {}

.babelrc

{
    "presets": ["es2015"],
    "plugins": [
        ["babel-plugin-transform-builtin-classes", { "globals": ["Array"] }]
    ]
}

test

$ babel test.js > temp.js
$ node ./temp.js

output

temp.js:1
(function (exports, require, module, __filename, __dirname) {  ✔ builtin extends patched
                                                               ^

SyntaxError: Invalid or unexpected token
WebReflection commented 6 years ago

good point ... however, the info was there for a reason ... do you mind changing it to:

if (name && process.argv.some(a => /^-v|--verbose$/.test(a))) console.info('...')

so that it's posible to have the same output as before ?

chocolateboy commented 6 years ago

Why is it there (and why would process be defined)?

WebReflection commented 6 years ago

is there for visual feedback, and process is always defined in node ?

chocolateboy commented 6 years ago

Doesn't babel run in the browser with a <script> tag? e.g. https://www.npmjs.com/package/babel-transform-in-browser

Who is the visual feedback for? Surely it's more important that the plugin works?

chocolateboy commented 6 years ago

From my experience so far, it's pretty clear the plugin works (without this message) :-)

WebReflection commented 6 years ago

on the browser, there is no issue with current code because console.info will just print info in the console.

Who is the visual feedback for?

developers. The plugin requires manual adjustment, it's easy to copy and paste a .babelrc and never patch the desired class. It happened to me already twice.

Surely it's more important that the plugin works?

the plugin works already if you use --out info via babel-cli.

if the issue is process then:

if (
  name &&
  typeof process === 'object' &&
  process.argv.some(a => /^-v|--verbose$/.test(a))
) console.info('...')

it's tooling, there's no need to save bytes here but I want to save my visual helper.

chocolateboy commented 6 years ago

it's easy to copy and paste a .babelrc and never patch the desired class.

Surely this is caught by tests?

At any rate, thanks for explaining your rationale. I can understand why that might be useful to you personally, but:

I'll stick with my fork for now and look forward to your (excellent) work being integrated in Babel.

Feel free to close this PR if you don't plan to remove the message.

WebReflection commented 6 years ago

it results in broken code

no, it doesn't. I've proposed an opt in, not an opt out. If you opt in, you know what you are doing.

it requires users of the plugin to somehow discover this issue to work around it

there is no issue. It's an opt in that can be documented as such

chocolateboy commented 6 years ago

FYI:

$ babel --version
$ 6.26.0 (babel-core 6.26.0)

$ babel --verbose test.js > temp.js
error: unknown option `--verbose'

$ babel -v test.js > temp.js
error: unknown option `-v'
WebReflection commented 6 years ago

latest versions should work for everyone, described in here: https://github.com/WebReflection/babel-plugin-transform-builtin-classes#about-logifpatched-option

chocolateboy commented 6 years ago

Thanks.

Please note if you are piping babel output directly the log will interfere with the produced output.

There's no need for the messages to interfere with the generated output if they're logged to stderr e.g. via console.warn or process.stderr.write.

WebReflection commented 6 years ago

wouldn't that make the eventual build step fail? nope, checked

it also feels weird to warn or err something that is actually an info

WebReflection commented 6 years ago

OK, 0.6.1 uses warn so everyone should be very happy now