Update minifier.js to remove server and development code

[ramezrafla]

A very simple change inspired by https://github.com/ssrwpo/uglifyjs2 to remove server (Meteor.isServer) and development code (Meteor.isDevelopment) easily (and speeds up client-side execution by removing Meteor.isClient and Meteor.isProduction)

We do this by search-replacing first to get simple var names (UGLIFYJS_FALSE and UGLIFYJS_TRUE) for terser to replace during conditional compilation

Notes:

1. In addition to reducing JS load, removing server-side code is desirable for security purposes (and protecting server-side code as well)
2. Tested in production and works great

[benjamn]

You should actually just be able to put these expressions in the global_defs map below, so you don't have to use a regular expression, and the minifier can replace the actual expressions in the AST—much safer!

[ramezrafla]

Ok, creating a new PR

[ramezrafla]

I tried using global_defs but terser missed quite a few Meteor.isClient -- this method of doing manual replace prior to terser didn't miss any

Also, to avoid passing affected source to Babel in case of failure by terser, we created a separate variable

[jamesmillerburgess]

I like the idea of eliminating unused code, but I'm worried that these regular expressions are not accurate enough for a global replacement.

Maybe this is a contrived example, but the below code would be a false positive:

if(SpaceObjects.Meteor.isProduction)
  doSomething(); 

False negatives aren't so risky, but just for an example to show it can also happen:

if(Meteor['isProduction'])
  doSomething();

[ramezrafla]

@jamesmillerburgess

I agree that these regexp can be improved. Is the only concern in your view that we have someone being non canonical and creating objects with 'Meteor' as field? In some cases, we can't help people from hanging themselves.

I can work on the regex some more

[jamesmillerburgess]

@ramezrafla

I think there are other ways this can cause unexpected behavior. I just put the first example that came to mind.

Here is a more realistic one:

<p>Use Meteor.isProduction when you want code only to run in production.</p>

This react element could be in an application that teaches people how to use Meteor, but the static text string would get modified by the minifier with the proposed change.

In which cases was global_defs not working? Maybe it is a bug in terser?

[ramezrafla]

It's hard to say where terser failed as the code was minified (and frankly, didn't feel like trying to untangle), we had quite a few Meteor.isClient and Meteor.isDevelopment in our js output.

Indeed, there are many cases where regexp fails, I was oversimplifying as I was blindly looking at our app. Edge cases should be few, but a platform like meteor should take care of them.

It's more complex than I bargained for. Maybe just maintain our fork of the package ...

[benjamn]

Just to be perfectly clear: using a RegExp to find and replace member expressions is not acceptable in the final implementation of this functionality. It's not a question of improving the regular expression. String matching using regular expressions is simply the wrong tool for the job, especially since the minifier has access to the actual AST. If you want this to be merged, I would recommend finding an AST-based strategy for replacing these expressions (and figuring out why global_defs isn't working would be a great place to start). Also, I think this behavior should be opt-in rather than enabled by default, since this change has very real backwards compatibility risks.

[sakulstra]

Also, I think this behavior should be opt-in rather than enabled by default, since this change has very real backwards compatibility risks.

@benjamn just wondering, but why is that? Shouldn't removing isServer code from the client be save for all applications? What issues could we face here?

For us this change would be a quite big improvement (didn't measure, but I think roughly 10% of our client bundle is Meteor.isServer code).

[macrozone]

maybe facebook's prepack would be a nice solution to that (when it's ready someday...)

[nathan-muir]

I was having a look at this, and there is a great babel plugin babel-plugin-minify-replace [See example]

While not part of minification, it might make sense to configure this plugin as part of babel-compiler / ecmascript.

We could configure it to replace all the Meteor.isXX [1] with true/false based on the arch which any minifier could then clean up unused code.

Example setting that would replace all Meteor.isServer with true.

{
  "plugins": [
    ["minify-replace", {
      "replacements": [{
        "identifierName": "Meteor",
        "member": "isServer",
        "replacement": {
          "type": "booleanLiteral",
          "value": true
        }
      }]
    }]
  ]
}

[1] isProduction, isDevelopment, isClient, isServer, isCordova, isModern, isTest, isAppTest, isPackageTest

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[sebakerckhof]

@ramezrafla are you still interested in working on this? Otherwise I'd be willing to continue where this left of. It still would need another solution than the regex replacement. Preferably with terser alone, if that's not possible then what @nathan-muir suggests sounds promising, but performance impact should be looked into.

[ramezrafla]

@sebakerckhof, I am really sorry for delay.
Got super busy and was travelling
My solution to maintain a local copy of the minification package became cumbersome to maintain, however, it's on the Meteor roadmap. I expect to be released soon. Is it ok if I close this issue?

[ramezrafla]

Oops -- sorry. I didn't realise this feature would be community-driven.
Yes I am interested in pushing it further if it is a community-driven effort. Terser should be the way to go. If no one took it on, I can go back to my testbenches with Meteor 1.9

[filipenevola]

The CLA is still not signed here, because of that I'm closing this PR. @ramezrafla and @sebakerckhof feel free to continue the work in a new PR as we can't merge this one without the CLA.