leongersen / noUiSlider

noUiSlider is a lightweight, ARIA-accessible JavaScript range slider with multi-touch and keyboard support. It is fully GPU animated: no reflows, so it is fast; even on older devices. It also fits wonderfully in responsive designs and has no dependencies.
https://refreshless.com/nouislider/
MIT License
5.7k stars 657 forks source link

Breaking changes: move functional styles inline #1167

Open mihkeleidast opened 3 years ago

mihkeleidast commented 3 years ago

First, a big thank you for this module. It's really helped out a lot over the years.

An issue rose from the last minor update:

Fixed: Compositing issues in Safari (note that this updates the stylesheet!) (#987, #998, #1029, #1154);

This changed the stylesheet, which is a breaking change - any libraries wrapping nouislider with their own stylesheet, but installing it via a transitive dependency, were broken. So if my lib is using ^15.4.0 and my lib is installed in a project, npm will install 15.5.0 in the project, misaligning the styles and breaking the project.

So, yeah - does noUiSlider follow semver? Should any libs that use caret to mark dependencies or something else (~15.4.0 for example)? Or was this change just not considered breaking, though it came out to be?

leongersen commented 3 years ago

Thanks for your kind words, I'm happy to hear the library has helped you.

Yes, noUiSlider follows SemVer. The problem here is the stylesheet: practically every change there is potentially breaking for everyone with overriding styles. When importing the compiled script and stylesheet without overrides, there are no breaking changes within major versions.

There's basically two parts to the stylesheet: the functional styling and the visual styling. Ideally, the functional part (which had a change in 15.5.0) would be considered @internal and overriding it would not be covered by BC-compatibility. There's no good way to annotate this, and no way to validate/enforce this while using an external style sheet.

Another approach would be to move the functional part of the stylesheet into the JavaScript. This would be a bit of an endeavor and a breaking change in itself, but it might be worth doing to avoid issues like this. What do you think? I'm very open to feedback in this area.

mihkeleidast commented 3 years ago

Yeah, totally understand that this is complicated and not very straightforward.

Regarding the specific circumstance, this had a change in both script and style, which can break more than some other changes. That's why I would have considered this a breaking change on it's own.

I think the specific width style could be moved to be inline on the element (the transform that also uses the multiplies already is), so if the multiplier should change further, the inline style would always trump whatever the user has provided in their CSS file. Not a fan of inline styles, but when working with these kinds of plugins, they're basically unavoidable. So yeah, I guess I would be for pulling the functional styles into JS in this case.

Aareksio commented 3 years ago

Hey! I stumbled upon the same issue updating dependencies. Since the stylesheet provided by nouislider includes not only the functional part, but also actual styles, we decided to extract the functional part ourselves. This led to display issues (customer-reported):

image

I am personally against moving the styles to JavaScript, but perhaps you could consider allowing import of only the functional part? This way projects which want full control over the style without worrying about overrides could import just the functional part you treat as not necessarily backwards-compatible.

- import 'nouislider/dist/nouislider.css';
+ import 'nouislider/dist/nouislider.functional.css';

This change would be non breaking, cheap to implement and opt-in, as nouislider.css would still be shipped as it currently is.