Replace remove() with dangerouslyRemove() #4
Conversation
|
Hey! This was on my list, thanks for this PR! |
| @@ -15,7 +15,7 @@ module.exports = function(precompile) { | |||
| throw file.errorWithNode(node, msg); | |||
| } | |||
|
|
|||
| this.remove(); | |||
| this.dangerouslyRemove(); | |||
There was a problem hiding this comment.
Do you think it makes sense to make this backwards compatible? Because if only the babel-plugin-htmlbars-inline-precompile package is updated, but the pugin is used with an older version of babel (dangerouslyRemove has been introduced in v5.5.0), an error would be thrown, since only the old remove would be available ...
What do you think?
There was a problem hiding this comment.
Yes, that's a very good point. I hadn't thought about all the transitive dependencies, but a quick grep through a recent Ember project suggests that it broccoli-babel-transpiler is the package which has the dependency on babel-core, and it's only ^5.0.0.
I've pushed a revised commit. Not sure how you'd prefer to do the backwards compat check so I've used typeof, let me know if you'd like a different approach.
| if (typeof this.dangerouslyRemove === 'function') { | ||
| this.dangerouslyRemove(); | ||
| } else { | ||
| this.remove(); |
There was a problem hiding this comment.
This looks good! One tiny nitpick: can you add a comment stating why we are doing this and maybe that this can be removed once we drop support for babel < v5.5.0... ?
There was a problem hiding this comment.
Oh, and can you change the indentation to two spaces instead of four?
|
This looks good!! I left a comment, would be cool if you have time to address this, otherwise I will merge this later today and do it later. Then you are still cool 😉 . Thanks again for this! |
Fixes this deprecation warning from babel-core: > Trace: Path#remove has been renamed to Path#dangerouslyRemove, > removing a node is extremely dangerous so please refrain using it. In this case it's not dangerous, because the module being imported doesn't actually exist. Checks for the presence of dangerouslyRemove() before calling it, falling back to remove() to allow backwards compatibilty with babel older than 5.5.0.
|
Done! It gave me a chance to also fix a typo in my commit message 😊 |
Replace remove() with dangerouslyRemove()
|
Won der ful! 😍 🍻 Cheers! |
|
This has been released upstream in |
Fixes this deprecation warning from babel-core which appears on build:
I think that in this case it's not dangerous, because the module being imported doesn't actually exist.
Thanks for ember-cli-htmlbars-inline-precompile, we're using it a ton and it's really useful ❤️