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

Handle export enum incorrectly #4291

Closed
wqcstrong opened this issue Dec 3, 2021 · 14 comments
Closed

Handle export enum incorrectly #4291

wqcstrong opened this issue Dec 3, 2021 · 14 comments

Comments

@wqcstrong
Copy link

Rollup Version

2.60.2

Operating System (or Browser)

macOS Monterey

Node Version (if applicable)

16.13.0

Link To Reproduction

https://github.com/wqcstrong/export-name-from-module

Expected Behaviour

Don't import the unused module in the bundle and its behavior is like the module produce some side effect.

Actual Behaviour

import the unused module because export enum in the module

@wqcstrong
Copy link
Author

BTW, any other conditions result to the error bundle like above?
Thx~

@NotZoeyDev
Copy link

I've been having the same issue recently and it's quite annoying as I cannot seem to find a way to make it not happen.

@wqcstrong
Copy link
Author

I've been having the same issue recently and it's quite annoying as I cannot seem to find a way to make it not happen.

I assume that export enum syntax may be treated as produce side effects by rollup-plugin-typescript2, although the assumption is invalid in my reproduction demo. So this's not rollup issue~

@NotZoeyDev
Copy link

I've been having the same issue recently and it's quite annoying as I cannot seem to find a way to make it not happen.

I assume that export enum syntax may be treated as produce side effects by rollup-plugin-typescript2, although the assumption is invalid in my reproduction demo. So this's not rollup issue~

Actually it happens with rollup-plugin-esbuild too which is what I'm having problems with at the moment!

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Feb 11, 2022

rollup only cares about the generated javascript, which in the case of your Test enum is being transpiled to:

https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBMAdgVwLZwCrAM7wN4CwAUHHIgIarDEC+QA

export var Test;
(function (Test) {
    Test[Test["name"] = 0] = "name";
})(Test || (Test = {}));

you should rather use Objects as enums or union types instead. enum is not (yet) part of ecmascript, but there's hope: https://github.com/Jack-Works/proposal-enum

@wqcstrong
Copy link
Author

rollup only cares about the generated javascript, which in the case of your Test enum is being transpiled to:

https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBMAdgVwLZwCrAM7wN4CwAUHHIgIarDEC+QA

export var Test;
(function (Test) {
    Test[Test["name"] = 0] = "name";
})(Test || (Test = {}));

you should rather use Objects as enums or union types instead. enum is not (yet) part of ecmascript, but there's hope: https://github.com/Jack-Works/proposal-enum

Why this generated Test not removed by tree-shaking?

@wqcstrong
Copy link
Author

Following is the expected interation behavior between rollup and plugins:

  1. rollup resolve the Button.ts source id
  2. typescript plugin transforms the content and return the result:
var Button = function Button() {
  console.log('Button');
};
var Test;

(function (Test) {
  Test[Test["name"] = 0] = "name";
})(Test || (Test = {}));

export { Button, Test };
  1. the Button and Test is not used in rollup module graph and can be removed with tree-shaking
  2. remove the import statment in importer file after source module parsed

@wqcstrong
Copy link
Author

@dnalborczyk what I missed🤕 , please give me some helps, thx~

@dnalborczyk
Copy link
Contributor

@lukastaegert could you shed some light on this?

@lukastaegert
Copy link
Member

lukastaegert commented Feb 12, 2022

Why this generated Test not removed by tree-shaking?

For performance reasons, when checking functions for side effects, they do not try out (yet) all possible parameter values they are called with, they just assume the worst, which is usually all parameters being undefined.

Improving here is something I wanted to do for a long time, but it is some very delicate mechanics with lots of things to consider, and there are more important bugs I need to work on at the moment.

The usual "fix" for these kinds of TypeScript issues is using treeshake.moduleSideEffects: true treeshake.moduleSideEffects: false (or use a function if a blanket false is too much). Basically this tells Rollup: Only look for side effects if you include something from the module. A good default for most well-written modules.

@wqcstrong
Copy link
Author

Why this generated Test not removed by tree-shaking?

For performance reasons, when checking functions for side effects, they do not try out (yet) all possible parameter values they are called with, they just assume the worst, which is usually all parameters being undefined.

Improving here is something I wanted to do for a long time, but it is some very delicate mechanics with lots of things to consider, and there are more important bugs I need to work on at the moment.

The usual "fix" for these kinds of TypeScript issues is using treeshake.moduleSideEffects: true (or use a function if a blanket true is too much). Basically this tells Rollup: Only look for side effects if you include something from the module. A good default for most well-written modules.

Thanks for your answer, I'll take a try later.

@lukastaegert
Copy link
Member

Ah sorry, it would be treeshake.moduleSideEffects: false

@wqcstrong
Copy link
Author

Ah sorry, it would be treeshake.moduleSideEffects: false

@lukastaegert It's be ok, thanks for your answer again. I'd keep dive into it

@wqcstrong
Copy link
Author

@NotZoeyDev the advice from me is not to use enum for now. As above says that:

you should rather use Objects as enums or union types instead.

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

No branches or pull requests

4 participants