Use ES6 export syntax

[Andarist]

The only thing Im unsure about is how "main" field of bower.json should get adjusted.

[Andarist]

@dcousens @JedWatson friendly 🏓

[dcousens]

Thanks! But, I don't think this is necessary. It doesn't make things any simpler, and it doesn't resolve any issues.
Happy to hear any arguments against though :)

edit: downvotes! eep

[Andarist]

@dcousens I've created an illustration of the problem that this would solve, you can check this out here.

This is a project with a simple structure:

• index.js - importing and using A
• module.js - containing A and B, where only B uses classnames

classnames is still in the bundle, although it is not used by the "app". Current structure of this project prevents classnames from being tree-shaken.

[dcousens]

Current structure of this project prevents classnames from being tree-shaken.

The tl;dr being that non-ES6 imports stop tree-shaking? Well ok!
I think if you can minimize contain the amount of babel boiler-plate included in the repository, to say 1 file or script, for the dist/ builds, then I'd be OK with it.

As is, there is like 3 new dev dependencies, simply to account for bower users.

[Andarist]

The tl;dr being that non-ES6 imports stop tree-shaking? Well ok!

Yeah, that was the point of my PR - sorry for not explaining this before.

I think if you can contain the amount of babel boiler-plate included in the repository, to say 1 file or script, for the dist/ builds, then I'd be OK with it.

Im not sure what I could minimize more. Let's sum up the changes:

• rollup is used as single build script - it uses babel under the hood through a plugin
• there is a .babelrc file, but it's rather a common practice and a clear indication for users that this project uses babel (i'd advise to keep it instead of inlining this setting into a rollup.config.js)
• there is a simple prebuild command added that removes dist directory
• i want to run tests on the actual distributed files, that's why build is also run as pretest
• i have also added a prepublish script, so noone ever try to publish without building & testing by accident
• there are added bind and dedupe directories that act as "proxy" directories - they allow node and bundlers resolvers to chose appropriate file (because those dirs have package.json in them). This makes it possible to still use require('classnames/bind') etc

If you want I can create those "proxy" directories in favour of gitignoring them and building them automatically within build script. I can also make any other change regarding everything Ive done & listed above - but Im not sure what exactly you'd like to have refactored, I'd love if you could clarify this for me 😃

As is, there is like 3 new dev dependencies, simply to account for bower users.

From my point of view devDeps are of no cost (except maintenance). I understand that not everyone agrees with that statement. That being said this PR does not target bower users, it targets most bundlers - webpack, rollup, browserify, etc - and for a library such as classnames I would say that this means targeting most of the users of this library.

[Andarist]

@dcousens 👋 could you specify what has to be done to get this merged in? :)

[dcousens]

@Andarist technically we shouldn't need to do anything but ES6ify the code.
The reason dist/ exists is to support Bower users AFAIK.

If you remove it, it may impact users who download/deploy from the GitHub repository directly.

If we drop the need for dist/, we can drop the rest too, only providing the exports as required.

[Andarist]

The reason dist/ exists is to support Bower users AFAIK.

You had no dist directory before my PR, so this:

If you remove it, it may impact users who download/deploy from the GitHub repository directly.

is not relevant here.

dist (or any other directory) was added so it can hold all distributable built JS files.

If one download/deploy directly from GitHub he/she shouldn't ever rely on master but pin its dependency to the exact commit/release. Moreover he/she shouldn't rely on package not having any built step - really most packages have it nowadays.

I haven't realized though that bower does not have its registry, but rather it uses git urls. I'm not sure how one is supposed to consume bower package - how one requires it etc. It seems that one of possible ways to do it listed in the docs is:

<script src="bower_components/jquery/dist/jquery.min.js"></script>

This would mean that ANY directory structure change is a possible breaking change for bower users (which I personally consider as internal detail, not part of the public API).

Because bower itself recognizes its tool as outdated (they have this text on their front page - "...psst! While Bower is maintained, we recommend using Yarn and Webpack for front-end projects read how to migrate!") I would consider 2 options:

• do not consider this as a breaking change
• release this as new major version & drop bower support (potentially add it later if somebody sends a PR for it)

I can tweak this PR however you want, but I'm still unsure about exact steps that you'd like to see taken here. I can add dist to the git, but it will bloat git's history quite a bit - it is a directory containing generated (so kinda duplicated) files.

[dcousens]

You had no dist directory before my PR, so this:

Oops, thanks.

IMHO, I'd rather drop Bower, but, I think this is up to @JedWatson now. I don't think I'd want to force a dist/ as is, ES5 isnt' dead yet, and the advantages of tree shaking (for this module) are probably not enough to warrant increasingly the dist complexity... yet.

Personally, I'd probably drop the AMD exports, solely require-js, and then let the rest figure itself out, with ES6 when ready (probably when Node is ready).

[Andarist]

IMHO, I'd rather drop Bower, but, I think this is up to @JedWatson now.

👍

I don't think I'd want to force a dist/ as is, ES5 isnt' dead yet

This actually is not forcing anything on anyone, dist is just an implementation detail here (internal directory structure). Standard main in package.json, which is well understood by node, bundlers and other tools let's your change what is considered by main entry point of the library - if one was using require('classnames') he/she will still use the same require call if this PR gets merged in. This PRs is intent to support various "targets" - node, cjs-aware bundlers and es-aware bundlers, all at the same time, without breaking anything (except bower ). From user's perspective this should behave just the same - only better in regards of tree-shaking etc.

[drewpc]

[edited to clarify my support for this PR]

@Andarist - great job refactoring to use ES6+ features! This strategy is perfect for supporting new syntax without dropping support for people who need older syntax. I definitely would prefer using classNames as an ES6 module. For those not familiar with the strategy for supporting CJS and ESM at the same time through NPM, below is a reference that talks about how to do this:

https://medium.com/webpack/webpack-and-rollup-the-same-but-different-a41ad427058c

[Andarist]

@drewpc this PR targets both CJS and ESM consumers already, everything should be covered 😃

[teameh]

+1 This would add autocomplete and auto importing in all IDE's of the IntelliJ family (WebStorm, PHPStorm etc.)

[doxiaodong]

For #167

[Andarist]

@doxiaodong ur solution is really more of a hack than a fix for this whole issue

[drewpc]

@JedWatson @dcousens Is there any further movement toward approving this PR? This is really the way JS libraries should be published going forward.

[dcousens]

Can this PR be re-based?

What is the cost of dropping rollup and using babel directly?

I don't know how damaging this will be overall seeing as Bower is deprecated, but I'm willing to deal with the nightmare if necessary.

[Andarist]

Can this PR be re-based?

Sure, gonna do that some time later & ping you when I'm done.

What is the cost of dropping rollup and using babel directly?

Babel by default (when using core plugins, not community ones) won't output module.exports = classnames for a single default export, it will rather output module.exports.default = classnames - mainly because interoperability between ESM & CJS is not trivial. Rollup chose developer experience in this regard, this means that:

// with rollup
var classnames = require('classnames')

// with babel
var classnames = require('classnames').default

Also it's IMHO easier to setup rollup with different module outputs (esm, cjs, umd) than in case of babel alone.

If we drop using const/let directly we could also drop babel entirely and just let rollup output appropriate module format - without transpiling rest of the file.

[dcousens]

@Andarist like you, I don't like the present state of the module export handling, however, this still doesn't feel "production ready" to me.
The fact there is still some differences in how the imports are coming in just doesn't seem like we should make the breaking change just yet. But maybe I am being too conservative?

I personally use ES6 everywhere, and then I let babel/browserify handle it all at the top level. But, this package is used, a lot, and by so many different use cases, I can't see this transition happening smoothly... yet. Maybe in 6-12 months we'll have progressed even further?

[thatbraxguy]

@dcocchia would making this a major version change be sufficient for not breaking it for current users?

[TrySound]

@dcousens Why not just make a major release? It won't break anything. This module is too small to be a dependency bloat.

[dcousens]

@Andarist wouldn't the present approach prevent tree shaking as it compiles down to ES5?

[TrySound]

ESM bundle will be treeshaken. CJS will be used by node and other who does not support module field

[Windvis]

What is the current status of this? Any blockers? Would love to see this merged.

[westbrook-asapp]

Would be great to see this become a part of the project!

[dcousens]

Marked as a breaking change.

[Andarist]

Do you plan on merging this? I could rebase to make it mergeable.

[FDiskas]

Still not merged?

if I use classnames as a dependency in my shareable components I can't bundle it - and this pr will help me

[!] Error: 'default' is not exported by src/components/node_modules/classnames/bind.js, imported by src/components/Badge/BadgeComponent.tsx
https://rollupjs.org/guide/en/#error-name-is-not-exported-by-module
src/components/Badge/BadgeComponent.tsx (2:7)
1: import * as React from 'react';
2: import classNames from 'classnames/bind';

[TrySound]

Try https://github.com/lukeed/clsx. Though there is no bind support.

[FDiskas]

@TrySound thank you. Actually binding not a big deal
doing like so

const className = cn({
        [styles.outline]: true, // Outline is default
        [styles.active]: !!active,
        [styles.focused]: !!focused,
        [styles.primary]: color === 'primary' || color === 'info',
        [styles.error]: color === 'error',
        [styles.success]: color === 'success',
    });

[JedWatson]

Just wanted to note that I'm about to revisit this package and publish a new major version with esmodule support and some performance improvements that have been contributed.

Sorry for the lack of updates until now, but the js ecosystem has come a long way since this was last seriously reviewed and things are different now so I'm going to dust it all off and do something about improving it.

[jonkoops]

ES module support has been implemented on the next branch, this PR's work could be based on that. pinging @Andarist

[Andarist]

From what I see, what's on the next branch is OK. I think it's time to retire this PR 😅