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

Possible Babel error with undefined variables? #8704

Closed
jasongrishkoff opened this Issue May 18, 2017 · 9 comments

Comments

@jasongrishkoff

jasongrishkoff commented May 18, 2017

This error has popped up after upgrading from 1.5-beta.10 to 1.5-rc.5.

The following code works fine on development, but in production mode, the var showDashboard defaults to true, even if it isn't an admin.

var showDashboard
if (isAdmin()) var showDashboard = true

I've been using this approach in my entire codebase -- roughly ~500 instances. Perhaps it was a really dumb move on my part, but ... the fact that it doesn't cause issues in dev, but does cause issues in prod, seems to imply something going on with the minification?

@benjamn

This comment has been minimized.

Member

benjamn commented May 18, 2017

Are you sure there's nothing wrong/different inside the isAdmin function? I wasn't able to reproduce this with just that code.

@jasongrishkoff

This comment has been minimized.

jasongrishkoff commented May 18, 2017

It's for any variable, no matter how vague.

var premium
if (blog.premium) {
  console.log("It's premium!")
  var premium = true
}

If blog.premium is true, the console.log fires and everything goes smoothly. If it's false, the log doesn't fire, but premium still returns true. I've had to go through and rewrite those ~500 instances where I've used this approach so that the initial declaration includes " = false" -- and in almost every case the variable was different and dependent on a different scenario.

@benjamn

This comment has been minimized.

Member

benjamn commented May 18, 2017

Do you have any custom .babelrc plugins?

@jasongrishkoff

This comment has been minimized.

jasongrishkoff commented May 18, 2017

@benjamn

This comment has been minimized.

Member

benjamn commented May 18, 2017

Inspecting minified code is always tricky, but the code for your application should be at the very bottom line of the minified bundle. You may also be able to click the {} button in Chrome DevTools to reformat the minified code a bit. If there's any way to figure out what code is being generated for this syntax, that would be helpful!

@hwillson

This comment has been minimized.

Member

hwillson commented May 18, 2017

Hi @benjamn - here's a quick repro:

meteor create --bare --release 1.5-rc.5 testapp && echo "var a; if (0) var a = 2; console.log(a);" > testapp/main.js; cd testapp; meteor --production

then check your browser console. The above shows 2 when minified, undefined when not. The minified code looks like:

var require = meteorInstall({
    "main.js": function() {
        var e;
        if (0) var e = 2;
        console.log(2)
    }
}, {
    extensions: [".js", ".json"]
});
require("./main.js");

It looks like the minifier (or something else) is getting confused by the double var. Removing the second var works properly (var a; if (0) a = 2;), so it looks like a hoisting issue.

@jasongrishkoff

This comment has been minimized.

jasongrishkoff commented May 18, 2017

@hwillson thanks -- that sounds like the issue

@benjamn

This comment has been minimized.

Member

benjamn commented May 27, 2017

Looks like this can be fixed by upgrading uglify-js to version 3.0.x (most recent: 3.0.12). Any objections to that upgrade, @abernix, if we keep the Babili fallback for now?

@benjamn benjamn self-assigned this May 27, 2017

@abernix

This comment has been minimized.

Member

abernix commented May 30, 2017

No objections here. We should probably do this in order to align with the (eventual, hopefully soon) move to uglify-es (See #8698) which uses the v3 API as well. The 3.0.0 version appears to have forked off ~2.8.22 and they've been consistently cherry-picking their PRs into both v2 and the new v3 branch.

Sadly, the migration to v3 of UglifyJS is not super well-defined but it appears Meteor's use of the API is simple enough and @sebakerckhof has the right changes in the aforementioned PR already (based on my own investigation as well). Also, since we don't currently allow options to be passed through (#8665) we shouldn't need to be concerned about those API changes.

@benjamn benjamn closed this in 8ff00a2 May 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment