Skip to content
This repository has been archived by the owner on Nov 30, 2023. It is now read-only.

The terser function should be a default export #7

Closed
vimtaai opened this issue Sep 16, 2018 · 20 comments
Closed

The terser function should be a default export #7

vimtaai opened this issue Sep 16, 2018 · 20 comments

Comments

@vimtaai
Copy link

vimtaai commented Sep 16, 2018

Hi!
I think it is strange behaviour that this plugin exports the plugin function as a named export as most rollup plugins (e.g. the official ones from the rollup github account) use a default export.
It just looks strange when importing modules that different plugins have to be imported in different ways.

import json from 'rollup-plugin-json'
import commonjs from 'rollup-plugin-commonjs'
import resolve from 'rollup-plugin-node-resolve'
import { terser } from 'rollup-plugin-terser'
@TrySound
Copy link
Owner

TrySound commented Sep 16, 2018

I don't believe default exports was a good idea in esm and never use them anymore (except rollup config).

All my plugins use named export.
https://github.com/TrySound/rollup-plugin-uglify
https://github.com/TrySound/rollup-plugin-size-snapshot

@TrySound
Copy link
Owner

TrySound commented Sep 16, 2018

By the way named export would prevent you from implicit renaming plugin.

import nodeResolve from 'rollup-plugin-node-resolve'

@vimtaai
Copy link
Author

vimtaai commented Sep 16, 2018

I see your point. In ESM I usually do both default and named exports. I accept your reasoning. Maybe rollup should change their export style? 😃
Also I just checked, rollup.config.js can import ES6 modules without problem. Maybe you could change the format of the plugins to ES6 module format and have a default and a named export? (or both esm and cjs, by using the main and module fields in package.json)

@TrySound
Copy link
Owner

No, providing both leads to api inconsistencies.

@TrySound
Copy link
Owner

By the way rollup itself exports as named export

import { rollup } from 'rollup';

@vimtaai vimtaai closed this as completed Sep 16, 2018
@eddiemonge
Copy link

Named exports are so much more of a pita. Using them means I have to use that name or add more code to use something else:

import { terser as IWantThisNameInstead } from 'rollup-plugin-terser'

instead of the much simpler (and more readable)

import IWantThisNameInstead from 'rollup-plugin-terser'

One reason named exports are better is that they explicitly state whats being used instead of inferring. But if you go with the one export per module theory, that point is moot.

For consistency, this just looks weird in a rollup.config.js file as being the only named export

@TrySound
Copy link
Owner

That's the point. If you want to have some weirdly named thing then you should explicitly rename it with as keyword.

For consistency with rollup itself (import { rollup } from 'rollup) I would change all plugins to named exports.

I understand your confusion but the feeling that default export is necessary is just a habit. Omitting it everywhere makes less problems for interop with cjs and one less thought how api should be provided.

@vladshcherbin
Copy link

@TrySound can you export both named and default functions?

I understand and respect that you like named functions, but when I have a list of plugins in my rollup config, a single import { terser } ... looks horrible.

I also tried rollup-plugin-size-snapshot and saw there import { sizeSnapshot } ... which already was "wtf is going on".

Rollup docs has an example of plugin and it uses default export, almost all other rollup plugins use default exports, can you please follow this convention?

Thank you

@TrySound
Copy link
Owner

@vladshcherbin Seriously? Two curly brackets doesn't make import horrible.

Well, I disagree with default exports. This is the reason I use named exports and the reason I'm not gonna encourage their usage. And I'm not gonna add the second way of doing the same thing. It's bad style of api. It's the same as import React, { Component } from 'react';. This is really horrible and obviously after esm support there will be deprecating of default export because there should be a namespace. My point is that module is a namespace even with single export.

Rollup documentation cannot dictate how I export my library. Moreover

import { rollup } from 'rollup';

@vladshcherbin
Copy link

@TrySound yes, it looks horrible when I have like 10+ plugins and all of them look the same (default exports) and only 1 is with curly braces just because someone wants to be special.

Here is a screenshot from the docs:

rp

The plugin there uses default export, same as plugins under rollup org.

As per { rollup } import - there are other exports besides rollup (watch, etc), of course it will be exported as a named function. Also, { rollup } is not a plugin and your package is.

So, while you don't like default exports, the whole rollup community uses them. It would be really nice to stick to that convention too. Please :)

@TrySound
Copy link
Owner

This is my choice. It does not introduce significant complexity. Respect it please.

@vladshcherbin
Copy link

@TrySound man, I respect your choices. I just like things to be consistent, besides { terser } import gives me a feeling that there is something else in the package while there is one single function.

I really don't want to create another rollup terser package just to get default export, is it really that hard for you to make a few people happy by adding default export?

Sorry for disturbing, just asking one last time. 🙏

@TrySound
Copy link
Owner

Yes. It's hard. Because I don't agree with default exports. I don't want people use it. This plugin is consistent with my opinion.

Which plugins do you use by the way?

@vladshcherbin
Copy link

@TrySound I was using rollup for server bundles, wanted to try bundling client js with rollup instead of webpack. Here is the list of server plugins I use:

rs

@vladshcherbin
Copy link

@TrySound as you can see, these plugins were built and are maintained by different people, but all of them have default exports. If I add here client plugins (for css, svg, etc), the list will be even bigger with only terser using named imports which kinda breaks the consistency and visual satisfaction for me.

This is the reason I found this issue while trying to create a new one and why I'm asking for it so annoyingly :)

@eddiemonge
Copy link

Everyone, this is going nowhere. As much as everyone here (I don't think I saw anyone else supporting named exports....) disagrees with this choice, it is @TrySound's plugin. It is not an official rollup plugin and thus does not have to conform to any of its standard or consistencies or well-established best practices. The great thing about this repo/plugin is that it is MIT licensed meaning you can fork it and export a default in the fork, following the MIT license rules. Then something like this can be done:

import terser from 'rollup-plugin-terser-default'

@TrySound
Copy link
Owner

@eddiemonge Why? It's just stupid import. It makes sense to me but it doesn't worth creating new package.

@TrySound
Copy link
Owner

It also will not be ethical of you.

@vladshcherbin
Copy link

@TrySound this is how people love consistency/default exports, man :)

Either me, or @eddiemonge , or someone else will do it, it's just a matter of time. That's why we asked you to add a default export so we don't have to pollute npm with the same package.

@eddiemonge I thought of a dependency on this package with 1 import and 1 default export, so we don't even need to copy/paste code or fork. But I still hope @TrySound will change his mind so we don't have to do such things :)

@TrySound
Copy link
Owner

This is becoming a senseless bikeshedding which I don't have a time to continue. You know my point.

Repository owner locked as resolved and limited conversation to collaborators Feb 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants