BootstrapMixin solution for ES6 Classes

[mtscout6]

Seeing as mixins are not supported with ES6 classes, we need to figure out a different way to handle what's going on in the BootstrapMixin.

As much as I love that I don't need to declare the three PropTypes bsClass, bsStyle, and bsSize they are not a good one size fits all solution. For example the Button component does not have css defined for tabs or pills which this implementation will allow. However, the current solution allows you to easy impose your own custom rules into the validation check; see #306, #440, and #496. @trevorr has done a great job with this so far and I'd still like to be able to support something like this, though I imagine it will get more complicated.

The current getBsClassSet function shouldn't be too diffucult to extract to a purely static function without any dependency on this so I'm not too concerned with that.

This is an issue that needs to be figured out sooner rather than later to continue allowing us to use ES6 Classes. (For example lack of support for this is blocking suggestions such as those proposed in #626 (comment))

[taion]

My 2 cents, I think the cleanest way to do this is to add a class decorator that attaches the additional props types. Something like:

@BootstrapProps
class Label extends React.Component { ... }

The syntax is a little bit unfamiliar because decorators are fairly new, but hopefully with time it will look normal/acceptable.

[taion]

That said, there's nothing wrong with ES5 classes + mixins. Facebook still use that approach internally, per the comment on: https://www.codementor.io/ama/questions/4965867003/how-soon-after-a-react-update-does-facebook-update-their-react-dependent-projects

[mtscout6]

The decorator still does not address the concern about the Button component not having support for tabs or pills, though still an interesting idea to think about.

[AlexKVal]

I also don't see any issues with using existing React.createClass function
with mixins support with ES6 code.

Facebook

We just don't use the ES6 class syntax for anything that requires a mixin.

It is an official API for near future.

When Facebook (and community as a whole) finds new the right way instead of mixins,
then it will become feasible and clear how to approach.

I made a research about current - ideas - instead of mixins.
There are no any prevalent yet.

They all have cons and pros and no one of them completely solves the issue with mixins with ES6 classes.

My vision of it:
pros:

1. React.createClass function is an official API
2. Facebook uses it also in their main products
3. We are not obliged in any way to refactor all components with ES6 classes
4. current solutions for mixins substitution with ES6 classes all have their problems
   (though as @taion aptly pointed it out: decorators are the most preferable way with the least downsides)
   cons:
   ??

In addition:
If we switch from mixins to a new "solution":

1. we will introduce new subtle bugs, because these are not well tested grounds yet.
2. by using newest technologies (like es7) we raise up entry barrier for new contributors
   (BTW it is already high enough 😉 )
3. by switching to use those (the cleanest way) decorators, or any other new solution,
   we expose ourselves to problems as in Implement new eslint rule object-shorthand #584 with linting of es7.objectRestSpread and so on.

What is the real benefit by using only and everywhere ES6 classes
instead of React.createClass function ?

It has to be heavy enough benefit to overweight all those points 😉

I propose to remove ES6 classes everywhere item from 1.0.0 Milestone \_O_/ 😄
Let's be pragmatic 🍒 🍒

[mtscout6]

While conversation about ES6 classes can be debated, the point that's getting missed is with regards to my original comment:

For example the Button component does not have css defined for tabs or pills which [the current] implementation will allow.

The BootstrapMixin is too broad of a solution to just apply to all components. What I'm proposing is that it would make more sense for it's functions to reside in utility methods, and let each component deal with it's PropType validations individually.

This discussion should not be about whether ES6 Classes or Mixin use is better, it is about enabling either for most components and fixing issues with our current Mixin that's prohibiting such.

On a personal note I'd like to try and use ES6 classes wherever possible since it's clear that at some point, when it makes sense to Facebook, that will be the supported approach. So changing now will help future proof us with React's changes.

[AlexKVal]

It seems today is not my day and I am very unfocused 😞
Sorry about that. 🍒
I really thought the question is about React.createClass vs ES6 class approach.

Of course I agree to try to simplify BootstrapMixin as much as possible and where it is appropriate
move away at all from it in some components.

[AlexKVal]

just for ref:

the current state of eslint linting of @decorators

error  Unexpected character '@'

Which can't not be removed even by using // eslint-disable-line on that line..

again: eslint doesn't support ES7 syntax.

eslint/eslint#2228

babel-eslint:
Using decorator doesn't count as usage by eslint babel/babel-eslint#72

... many others issues with pretty same content.

espree eslint/espree#116 is the blocker of this feature.

That's the extent to which we're going to support it in Espree until it's ratified in some standard.

eslint/eslint#1761 (comment)

This is purely dependent on the parser. When the parser supports it, so will we.

eslint/espree#140

[AlexKVal]

At least from now on we can try to use decorators
thanks to https://github.com/alexkuz/eslint-plugin-decorator

Disclaimer from author:

This plugin barely supports ES7 decorators - namely, it just allows you to get rid of no-unused-vars errors for variables used in decorators. That is, it's just a temporary solution, it doesn't check decorator content for validness or something.

[taion]

We're not actually using all the Babel optional transforms: https://github.com/react-bootstrap/react-bootstrap/blob/e1c6aa227c654dbc7a3db8e4e010dca9d6e22daa/.babelrc

[mtscout6]

I'm fine with using more if we need to, but some of them may have stability issues which we should be aware of. I used the one optional feature that the jsx tool from facebook was supporting and we needed to do the initial ES6 transformation.

[AlexKVal]

alexkval: At least from now on we can try to use decorators

I make mention of it because of

taion: My 2 cents, I think the cleanest way to do this is to add a class decorator
from here

As for the

mtscout6: but some of them may have stability issues which we should be aware of

I also would prefer not to use any of ES7 (let alone es7.objectRestSpread because of JSX).
I said all about it here already.
🙌 🌈

[AlexKVal]

it seems as if a year has passed since 😄

Because since v0.24 we use "stage": 1
https://babeljs.io/docs/usage/experimental/#stage-1
then, we can try to implement it with es7.decorators
and use https://github.com/alexkuz/eslint-plugin-decorator to deal with linting errors.

Update
alas eslint-plugin-decorator already doesn't exist.

[mathieumg]

use https://github.com/alexkuz/eslint-plugin-decorator to deal with linting errors.

404?

[AlexKVal]

Pff.. a couple of days ago it was existed. 😓

There is the https://github.com/babel/eslint-plugin-babel another promising one.
Maybe in the course of time it will implement and decorators linting 😄

[AlexKVal]

I'm on it. Will push a PR after v0.24 released.

[AlexKVal]

We should decide what a way to choose.

Now the default way to set CSS class for each component is through private API bsClass property.
Though it is private only in the current documentation.

It was always possible to set this property to any custom value and I'm afraid that there could exist some code that use it, though I presume those are edge cases and they do it wrong.

Now we have Modal sub-components with their own custom - public documented API - modalClassName, that is used instead of bsClass.
http://react-bootstrap.github.io/components.html#modals-props-modal-header
http://react-bootstrap.github.io/components.html#modals-props-modal-title
http://react-bootstrap.github.io/components.html#modals-props-modal-body
http://react-bootstrap.github.io/components.html#modals-props-modal-footer

If we decide that we allow users to set their own custom classes for our components,
then we should make the bsClass property a public and document it better.

Though: I believe this is the wrong way. Users should use bsStyle and className instead for customization.

In any case we should refactor those new Modal components to use bsClass instead of modalClassName.

I can do it, but should we add a deprecation warning for it or it's not so essential at this point ?

Thus. Two questions:

1. bsClass public or private ?
2. should we add a deprecation warning for removing the public modalClassName properties from Modal family ?

/cc @react-bootstrap/collaborators

[jquense]

I'm not sure we should remove those new props until there is a better solution for BootstrapMixin, as of right now it is really functioning as a private prop as far as I can tell, whereas the modalClassName props are meant to be public and solve a different use case than bsClass does.

As it is now there is no way that getBsClassSet() will work for the modal pieces, since the -header, -footer, -body and -title parts are not styles or sizes

[mtscout6]

One potential use case of a custom bsClass was proposed in #404, where someone wanted to essential style a modal as an aside. The DOM structure was identical the only difference is the class prefix.

[AlexKVal]

Ok then. At this stage we agree:

1. modalClassName - public and is a part of Modal.* API now.
2. bsClass - private

While I'm eliminating this Mixin, I see, that we can totally remove bsClass private property.
What do you think ?

[mtscout6]

I think my previous comment pointed at the contrary, bsClass should remain public. As I stated earlier #404 is a fine example of why it's beneficial to leave public.

[jquense]

I think it makes sense as a public api, but we really need to clean it up before I feel comfortable really telling ppl about it, since it works oddly right now. For all intents and purposes it is a prop and so a technically a public api, whether we want it to be or not, so we need to treat it as a breaking change if we change the behavior.

At to the earlier discussion if we are adding an api to add custom bsClasses then it really needs to be public ultimately, right?

[jquense]

which is all to say: some places we treat it like its private and others we don't

[AlexKVal]

I have no problem at all with bsClass being public. ❇️
(at least it is less work and we don't need deprecation step) phew.. 😅

Should we at some future point, when bsClass is already public and documented,
migrate (through deprecation) modalClassName => bsClass for Modals too ?

[mtscout6]

I had to change the react/prop-types eslint rule to warning this morning on the dropdown-revisisted branch for #526. I hope for this to be a temporary measure, but it was the best I could do with the dropdown buttons being pure es6 until we figure out this issue.

[AlexKVal]

No worries. I deal with it after the dropdown branch will be merged 😉
Thank you for the note about it.

[AlexKVal]

I would prefer to deal with that after #1096 and #962 are merged.
Otherwise it will be a bit of headache for all.

[AlexKVal]

#1257 is successor of this.