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

Add "default pure" configuration option #1443

Open
danieltroger opened this issue Sep 8, 2023 · 7 comments
Open

Add "default pure" configuration option #1443

danieltroger opened this issue Sep 8, 2023 · 7 comments

Comments

@danieltroger
Copy link

danieltroger commented Sep 8, 2023

Bug report or Feature request?

Feature request

It would be great to be able to configure that certain global functions will always be pure, such as WeakMap, Map, Set, etc.

When having a very large library and relying on tree-shaking so that people who only use a little of it only get a little in their output, it's a continuous hunt of adding pure comments to side-effects.

It's quite easy to track down where to add a pure comment for most code, but there are always a random 50 calls left to for example new Map that are hard to find the culprit for.

I think very very few people have a redefined Map function with side-effects so it would make things a lot simpler to just be able to say that certain functions are always pure.

Version (complete output of terser -V or specific git commit)

5.19.4

Complete CLI command or minify() options used

https://try.terser.org/

terser input

const test1 = new WeakMap();
const test2 = new Map();
const test3 = new Set();
class test extends EventTarget {
}

terser output or error

new WeakMap,new Map,new Set;EventTarget;

Expected result

Terser config:

{
  "alwaysPure": ["Map","Set","WeakMap","WeakSet", "BigInt", "Promise", "URL", "Symbol", "EventTarget"]
}

Output code:

@fabiosantoscode
Copy link
Collaborator

fabiosantoscode commented Sep 20, 2023

I think a good name for this would be assume_library, where true would mean that accessing globals that are known JS or browser library items is safe (IE, doesn't cause a crash related to an undefined variable being accessed). This would make them safe to remove.

edit: I checked that some of this is already implemented here. What needs to be added is a list of globals whose calls (and new's) can be assumed to be pure.

@ehoogeveen-medweb
Copy link

I don't know if it's a good fit, but maybe you could use the globals package and tie it to the specified environment. I guess the question then would be how you pass the environment - maybe assume_library can take an object like ESLint's env option.

@fabiosantoscode
Copy link
Collaborator

I still want to do this, but it will be technically incorrect. People can re-assign Map! Also, some environments will not have Map.

Even removing things like new Array() is hidden behind the unsafe option.

But on the other hand, only null and undefined are assumed to be "== null", which is also technically incorrect, and in some niche places property access is considered to be pure, which could be overridden by effectful getters.

On using the globals package: seems like a good idea. Unfortunately it doesn't specify which globals are functions, classes or objects, but I don't think that's such a big deal. If this would be an enabled-by-default option, I think it would use everything in globals.

@danieltroger
Copy link
Author

I still want to do this, but it will be technically incorrect. People can re-assign Map! Also, some environments will not have Map.

Great to hear! I think that very, very few people re-assign Map and similar globals, so assuming that it's pure would bring a net benefit, in my opinion.

As long as it's a feature I'm happy :)

@ehoogeveen-medweb
Copy link

Everything in globals would be too much, I think: You probably don't want everything in browser in a Node.js environment, and you probably don't want everything in node in a browser environment :)

@fabiosantoscode
Copy link
Collaborator

It's not like it would add any more code. It's important to make sure that something that would crash before minify will also crash after minify, but if "process" is found as a global variable access in some bundle, it's probably a remnant of some isomorphic code, and likewise for "window" in a node environment.

Making the option more specific, and letting you choose specific environments could be cool for the future, IE we could assume that the typeof Map is not undefined, and remove polyfills.

@fabiosantoscode
Copy link
Collaborator

It looks like I completely forgor about pure_funcs. If you pass the pure_funcs: ['Map', ...] compress option, it achieves the desired effect here, and removes the loose function names at least in a second pass.

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

3 participants