-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wrapped every log in process.env.NODE_ENV check for better minificati… #55
Conversation
…on. Also restructured build configs in the process.
@Andarist The PR is great (as usual) ...I don't think it can be merged as is because of where I accidentally mislead you. Reason:
I think if we make Can you make that update? ...Otherwise, I can fork your branch and submit a PR for you to review. Thanks SO much! You're fast! 🏎💨 |
I'm not sure what do you mean, could you rephrase & provide some sample code snippet backing what you describe? |
@@ -197,7 +210,9 @@ export class ChildAPI { | |||
} | |||
|
|||
emit (name, data) { | |||
log(`Child: Emitting Event "${name}"`, data) | |||
if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don’t we put the process.env check inside the log function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that we could have something like this:
this.debug = info?.debug // options operator
// ...
emit (name, data) {
if (this.debug) log(`Child: Emitting Event "${name}"`, data)
}
...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakiestfu @Andarist if Jacob gives a 👍 with this PR, let's do it!
I can fast provide a fast-follow with what I was thinking and then all of us can review that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakiestfu
that wouldnt work, it would just remove contents of the log
function, making it effectively a noop (although with some minification setups it could still leave access to Postmate.debug
as possibly it wouldnt know that this property access is side effect free). And this wouldnt change a thing. I can recheck it, but when I've last checked it static analysis performed by minifiers wasnt just good enough to know that call sites of such noop function can be removed.
By wrapping each call site in we ensure that they can be trivially removed, because those conditions will be constant and minifier will know that code cannot go into if (false) { //...
branch. And if w wrap every call site of this log
function minifiers will be able to remove it as well because it wont be referenced anymore.
@yowainwright
it would actually just move this.debug
/Postmate.debug
condition to other places and increase final bundle size a little, because you'd have to check this at each call site instead of single time in the log
itself.
@Andarist Lets wait for Jacob's review before merging. Thanks for the explanation! |
Sure thing, if I can do anything to explain this better, back with examples, whatever - just ping me about it :) |
@Andarist Great work. Is it possible to cache the environment variable higher in the scope? i.e. const isProduction = process.env.NODE_ENV === 'production'
if (!isProduction) { log('Example') } |
It is possible (remember that's it only possible if we use this 'cached' variable within a single scope, so do not ever export it), technique allowing this is called "constant folding". // input
const log = msg => console.log(msg)
const isProduction = 'development' === 'production'
if (!isProduction) { log('first') }
if (isProduction) { log('second') } // outputs
// uglifyjs -c
const log=msg=>console.log(msg),isProduction=!1;log("first");
// uglifyjs -c --toplevel
const log=msg=>console.log(msg);log("first"); I'm not sure which versions of UglifyJS can do that though, also I think Rollup doesn't do that (at least yet) where it would be able to remove dead branch if it would be "inlined" (in opposite to "cached"). As the current check is a little bit verbose other projects (i.e. React) introduce sometimes "build time variables" which they replace for any output type, so a single check looks like this: if (__DEV__) {
log('log me')
} and they replace |
Nice, @Andarist. Thanks for the knowledge, we'll get to merging this. |
…on. Also restructured build configs in the process.
fixes #54 closes #53
What has been done:
What could be done:
build
directory, it's a duplication and bloats PRs & commits with changes