Cannot use duplicate class keys when stacking theme classes

[tomdye]

Enhancement / Discussion

Code

import * as css from './styles/button.m.css';
import * as iconCss from './styles/icons.m.css';

@theme(css)
@theme(iconCss)
export default class Button extends WidgetBase<WidgetProperties> {

    protected render() {
        return v('button', {
            classes: this.classes(
                css.play,
                iconCss.play
            )
        });
    }
}

Expected behavior:

button should render with the play class from both the button.m.css and the icons.m.css files.

Actual behavior:

button renders with only the icons.m.css play class. The reverse lookup in Themeable cannot differentiate between the two stacked theme files.

Summary

this.classes should be able to differentiate between the different css files / stacked themes. It may be that we need to prefix or similar the reverse lookup map within Themeable.

[tomdye]

It's likely that the main time this issue will surface is in the case of iconCss. In most other cases it is the desired approach for @theme stacks to override one another. As such I believe the best approach is to suffix icon classes with Icon.

[agubler]

Indeed this is currently behaving as expected - stackable themes provide a mechanism to override themes provided by a base class when extending a widget.

We could consider enhancing @theme in a way that provides an option to tell it not to override any existing classes of the same name.

[tomdye]

@agubler I think that enhancing @theme in such a way would likely lead to further problems down the line. Perhaps we should leave it be.

[agubler]

@tomdye what further problems do you foresee?

[tomdye]

I meant that it might raise issues when some things are overridden and others are not. Creating a predictable end result may be difficult. Perhaps I'm wrong.

[agubler]

@tomdye if the default is to override and a consumer has to explicitly choose for them to be used additively, would that be known and predictable enough?

[tomdye]

what i meant is that someone could import three themes and set the middle one to be additive. Could get messy is all I mean. Plus I'm not entirely sure if we need it or not based on the only use case I can think of being icons.

[agubler]

I was just thinking that something like this could support the flexibility needed

function theme(theme: object, override: boolean = true) {
// stuff
}

And then a usage that could look like this, which is fairly intuitive:

@theme(css)
@theme(iconCss, false)
@theme(otherCss)

I'm not saying we definitely need an enhancement like this, but we certainly could support something for the scenario without it becoming problematic.

[sebilasse]

@tomdye Am I right that this does not happen when you dojo build webpack -w and watch it on http://localhost:9999 ? Again : I would prefer [name]__[local]__[hash:base64:5] for naming the classes because if the css name (or a hash of its absolute path) is part of the classname it would not happen (?)

[agubler]

@sebilasse this hasn't got anything to do with @dojo/cli-build, it's related to the reverse look up used by Themeable.

The class name pattern generated by css modules does not affect this.

[agubler]

@tomdye your thought are that we should leave the functionality as it is? Do you think we should close this issue as "won't fix" or leave it open for the future?

[tomdye]

Either option works. I can't think of am important enough reason to fix this at the moment but there's always the possibility that one will come in the future.

[tomdye]

I have come across a similar issue to this using widget extension which effectively stacks theme classes. In this scenario we would most certainly expect the extended theme class to override the original class (not to be added alongside it). As such I believe this issue should be closed and I shall raise a new issue for the new bug I have found relating to this. :)