-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Function calls with side effects are removed (when they shouldn't be) #476
Comments
@Pauan Thank you for reporting this issue. For reference, here is the offending code: const foo = {};
const set = (key, value) => {
foo[key] = value;
};
set("foo", 1);
set("bar", 2);
set("qux", 3);
console.log(foo); It's curious that the resulting core preserves the definition of Replacing the const foo = {};
function set( key, value ) {
foo[key] = value;
}
set("foo", 1);
set("bar", 2);
set("qux", 3);
console.log(foo); I suspect that Rollup preserves the definition of the arrow function above, as it believes that the definition itself somehow modifies |
Interestingly, commenting out #!/usr/bin/env bash
set -e
cat <<EOS > file.js
const foo = {};
const set = (key, value) => {
foo[key] = value;
};
set("foo", 1);
set("bar", 2);
set("qux", 3);
console.log(foo);
EOS
git clean -dfx test
npm install
npm run build
./bin/rollup file.js | grep qux
echo PASS |
Rollup is not acknowledging the side effect in the MemberExpression of the left side of the AssignmentExpression in function If the test case is modified as follows it works as one would hope: const foo = {};
function set(obj, key, value) {
obj[key] = value;
}
set(foo, "bar", 2);
set(foo, "qux", 3); The test above was was constructed after observing that a side effect is generated from a modifier node that happens to be a parameter: Line 97 in 4b61455
I think a fix has to be introduced in the general vicinity of this code to get rollup to recognize the side effect in the foo AssignmentExpression MemberExpression which is in the same module but outside the block scope of the function. |
The same reason why rollup presently (correctly) drops
to produce:
is also why rollup does not acknowledge a side effect in the
which ultimately causes the In both cases the condition below marked as
Lines 90 to 102 in 4b61455
Some code has to crafted to differentiate whether the declaration in question is actually used by the final program. |
Fix in PR #512 |
Awesome, thanks guys! |
Here is a reduced test case:
http://rollupjs.org/#%7B%22options%22%3A%7B%22format%22%3A%22amd%22%2C%22moduleName%22%3A%22myBundle%22%2C%22globals%22%3A%7B%7D%2C%22moduleId%22%3A%22%22%7D%2C%22modules%22%3A%5B%7B%22name%22%3A%22main.js%22%2C%22code%22%3A%22const%20foo%20%3D%20%7B%7D%3B%5Cn%5Cnconst%20set%20%3D%20(key%2C%20value)%20%3D%3E%20%7B%5Cn%20%20foo%5Bkey%5D%20%3D%20value%3B%5Cn%7D%3B%5Cn%5Cnset(%5C%22foo%5C%22%2C%201)%3B%5Cnset(%5C%22bar%5C%22%2C%202)%3B%5Cnset(%5C%22qux%5C%22%2C%203)%3B%5Cn%5Cnconsole.log(foo)%3B%22%7D%5D%7D
As you can see, it completely removes the calls to
set
In my real application, I am using an object to store configuration, and then I am using function calls to populate the object.
But rollup removes all of the function calls, so the configuration ends up being empty.
The text was updated successfully, but these errors were encountered: