Skip to content
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

Combined var statement with more than two vars doesn't work #2593

Closed
Deraen opened this issue Aug 1, 2017 · 3 comments
Closed

Combined var statement with more than two vars doesn't work #2593

Deraen opened this issue Aug 1, 2017 · 3 comments
Assignees

Comments

@Deraen
Copy link
Contributor

Deraen commented Aug 1, 2017

Related to #2450, #2579, 8fc8229

We just tested and debugged Closure-compiler master with ClojureScript with @anmonteiro and we found out that 8fc8229 breaks for example Lodash which worked previously. One of the problematic files is https://unpkg.com/lodash@4.17.4/_baseGetTag.js which has combined var statement with three vars. Only the two first vars are processed.

Here is a simple test case:

foo.js

var first = require('./bar'),
    second = require('./bar'),
    third = require('./bar'),
    fourth = require('./bar'),
    fifth = require('./bar');

module.exports = {};

bar.js

module.exports = {};

test and output

❯ java -jar target/closure-compiler-1.0-SNAPSHOT.jar bar.js foo.js --process_common_js_modules --module_resolution node
var module$bar={};
var module$foo={},
first$$module$foo=module$bar,
second$$module$foo=module$bar,
third=require("./bar"),
fourth=require("./bar"),
fifth=require("./bar");
@anmonteiro
Copy link
Contributor

I just bisected this to confirm and got the following output:

8fc82299c45b0a33f19834222acda4850fd77bc0 is the first bad commit
commit 8fc82299c45b0a33f19834222acda4850fd77bc0
Author: Chad Killingsworth <chadkillingsworth@gmail.com>
Date:   Fri Jul 28 10:01:09 2017 -0700
    Fix NPEs in ProcessCommonJSModules caused by multiple var declarations
    
    Closes https://github.com/google/closure-compiler/pull/2579
    
    -------------
    Created by MOE: https://github.com/google/moe
    MOE_MIGRATED_REVID=163482326
:040000 040000 3806b180cdbd083a13cb796ca0f301e0a126fb68 6e37afed8b90df0419675b5b5b1ca39114374ebd M	src
:040000 040000 cd993c58a32d7443fe72a561dd11c320b5842815 901881e4f217ef57cdcce3fb7dbdb35396c0b34b M	test

@Deraen Deraen changed the title Combined var statement with more than two vars doesn't work Combined var statement with more than two vars with require doesn't work Aug 1, 2017
@Deraen
Copy link
Contributor Author

Deraen commented Aug 1, 2017

In fact this is not related to require, here is even simplified test:

foo.js

var first = 1,
    second = 2,
    third = 3,
    fourth = 4,
    fifth = 5;

module.exports = {};
❯ java -jar target/closure-compiler-1.0-SNAPSHOT.jar bar.js foo.js --process_common_js_modules --module_resolution node
var module$bar={};
var module$foo={},
first$$module$foo=1,
second$$module$foo=2,
third=3,
fourth=4,
fifth=5;

@Deraen Deraen changed the title Combined var statement with more than two vars with require doesn't work Combined var statement with more than two vars doesn't work Aug 1, 2017
@Deraen
Copy link
Contributor Author

Deraen commented Aug 3, 2017

I can confirm this was fixed by #2596

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants