Add optional argument to withStyles allowing arbitrary data to be passed to the plugin

[fvgs]

I propose modifying withStyles to accept an optional data argument to be passed through to the plugin for the plugin's use. The data argument would be passed to the create method of the interface.

Implementing this as an arbitrary and optional data argument maintains backwards-compatibility and allows any plugin now or in the future to make use of the feature for its own purposes.

The use case on which I'm currently working benefits from allowing the user to pass an optional string to withStyles. The purpose of the string is to uniquely identify the component consuming the styles relative to other components also using withStyles.

[lencioni]

I'd like to understand your use case some more. We don't use the term plugin in this project, so I'm curious what you mean by that. Also, can you give some examples of what this would look like for consumers and any alternatives that you have considered?

[fvgs]

I'd like to understand your use case some more.

Many components can use withStyles within the same project. Likewise, a user can choose to use the same style name multiple times across different components that are using withStyles. This leads to possible collisions among style names in different components. Aphrodite, for example, addresses this issue by computing a hash to uniquely identify unique sets of styles. This works for aphrodite's use case. However, hashes are neither deterministic (they change when the styles change) nor are they expressive. This is not ideal for constructing CSS class names to be statically manipulated by users.

By allowing the user to provide a string (typically the component name) to the withStyles call, withStyles can pass that string along to the interface (what I was referring to as the plugin). The interface can then use that string as a namespace for constructing class names that will not collide with classes constructed by other components. Having that unique identifier (typically the component name) solves the determinism issue as well as the expressiveness issue.

We don't use the term plugin in this project, so I'm curious what you mean by that.

I wanted to make explicit the distinction when referring to the interface defined by react-with-styles for the interfaces (plugins) to implement, and the interfaces (plugins) themselves. I see we use the term interfaces in the readme. So this may not be the time to introduce new terminology. I just want to make sure it's clear when I'm referring to functionality that is offered by the interface react-with-styles defines, versus the individual interfaces, each of which may have its own independent set of concerns.

can you give some examples of what this would look like for consumers

// MyComponent.jsx
export default withStyles(myStylesThunk, 'MyComponent')(MyComponent);

// interface.js
function create(styles, data) {
  const stylesToClassNames = {};
  Object.keys(styles).forEach((styleName) => {
    stylesToClassNames[styleName] = `${data}--${styleName}`;
  });
  return stylesToClassNames;
}

alternatives that you have considered

It would be nice, for this use case, if it were possible to reliably extract the name of the component across different environments. However, as we've discussed, there does not appear to be a reliable method to do so.

The main criterion is being able to construct deterministic class names. This essentially requires reliance upon some artifact of the component, such as the component name. I think allowing the user to explicitly provide a unique namespace on a per-component basis will be less prone to issues. The explicitness prevents a situation where the user changes some property of the component (such as the component name) and inadvertently breaks the association of static CSS classes to that component.

[ljharb]

Is the name not something that could be inferred directly from the component? It would still need to be provided to the interface, but I'm concerned this would create a situation where every single call to withStyles is not identical which would defeat the purpose of making such components using withStyles 100% reuseable.

[fvgs]

Is the name not something that could be inferred directly from the component?

@lencioni can comment on this more in-depth. But the short answer is, not reliably. One of the issues is in the browser babel will have stripped away the name/displayName information which is necessary to construct the correct class names. Working around that imposes additional external requirements on the user's build process. It also implicitly assumes the user's component names are unique.

this would create a situation where every single call to withStyles is not identical

I agree this is an important concern as it harms interchangeability. The worst outcome with the current proposal is the user might have to add/change the second argument to all their withStyles calls when switching to a different interface.

---

I think the underlying issue is that of namespacing CSS, which is by no means a new problem.

Here are the three approaches I see:

Approach A

Require a unique string as a second argument to withStyles. Why?

Styles passed to withStyles, if not written in accordance with a naming scheme ensuring uniqueness across components, cannot be converted into class names that are both unique and deterministic.

Uniqueness + determinism is not a requirement of all interfaces (e.g. aphrodite). But it is a requirement for some interfaces (e.g. static CSS output).

This approach improves interchangeability of interfaces by requiring a unique identifier per component regardless of the interface. Thus, styles that work with the aphrodite interface but are not fully unique won't have to be renamed in order to work with an interface that requires the styles it's given be unique.

Approach B

Leave the interface as it is. Why?

If the user never uses an interface requiring unique style names be given to withStyles they will never run into an issue.

Likewise, if the user adheres to a style naming convention that ensures uniqueness of style names across components they will never run into an issue.

If the user writes non-unique style names and (possibly in the distant future) wishes to use an interface that requires unique style names be given to withStyles, they run into an issue. That is, some classes/styles will collide.

Approach C

Allow an optional, unique string as a second argument to withStyles. Why?

It's backwards-compatible. It doesn't force users to provide the argument to interfaces that don't need it. At the same time, if the user later chooses to use an interface that does require such an argument, they can add it to all their withStyles calls. It's not ideal, but is better than having to rewrite every style name.

Note that this approach is a narrower version of the initial proposal. The initial proposal created far more flexibility in the interface with regard to what arguments could be passed and thus would have harmed the general interchangeability of interfaces by encouraging interface-specific arguments.

---

With the goals of improving interchangeability of interfaces while maintaining uniformity across withStyles calls, Approach A is the one I find most appealing.

The arguments against Approach A are it introduces a major change and it requires slightly more input from the user per call to withStyles. In some cases, the interface won't make use of that information. However, it requires that information in order to abstract away and address an underlying problem with how CSS works. I think the overall effect is strongly positive.

[ljharb]

I think approach C is a non-starter for the reasons I mentioned - react-with-styles only works because components can always be written without the interface in mind, and it's every interface's responsibility to adapt to that.

With Approach B, would you be able to, at runtime, throw errors when collisions are detected?

[fvgs]

I believe you could. It would require implementing functionality to keep track of what class names have been exposed via a styles prop to a component as each component is accessed for the first time.

Additionally, presenting a warning/error when pre-compiling the static CSS would be trivial.

react-with-styles only works because components can always be written without the interface in mind

I think what we're finding is react-with-styles and existing interfaces support style names that are not guaranteed to be unique + deterministic across all components. However, some interfaces will work incorrectly without that requirement and (as far as we've found) have no way to adapt.

Thus, a user wishing to use the CSS interface we're discussing would have to write their component's styles with the interface in mind.

[ljharb]

If that's the case, then that sounds like something react-with-styles should try to fix internally, without having to require users provide additional information (if possible).

[fvgs]

That's definitely the ideal solution. I think it boils down to the following question:

Is there a deterministic + unique identifier associated with every component that can be reliably accessed on both the browser and server?

[ljharb]

perhaps its resolved import path, but there's no way to get at that unless it's provided manually or by a babel transform in the original code.

[fvgs]

@lencioni suggested a babel transform for preserving the name/displayName in the browser bundle. But similarly, it sounds like that would impose additional requirements on the user's build process.

[ljharb]

True. However, that seems like a reasonable requirement imo for debugging anyways.

[fvgs]

For debugging? I don't follow.

facebook/react#1137 has discussion on this topic but doesn't offer a solution. This feature has been requested for a while.

[ljharb]

Yes, component names could then be attached to stack traces, and show up in dev tools.

[majapw]

I am actually most the fan of option C. @ljharb can you elaborate a bit more on why it might be inherently bad to have withStyles calls not look identical if the aspects that are different are optional?

I think it would be helpful to talk more specifics in this scenario. When we move react-dates to rely on withStyles, if we were to follow current conventions, we might do withStyles calls like:

export default withStyles(() => ({
  container: {
    ...
  },

  container_vertical: {
    ...
  },

  ...
}))(DateRangePicker));

export default withStyles(() => ({
  container: {
    ...
  },

  container_vertical: {
    ...
  },

  ...
}))(DayPicker));

if we deterministically created class names from this object, we'd get two conflicting .container and .container_vertical classes that don't correspond to the same components/sets of styles. Even if we registered a namespace with the interface, we'd still have something like .react-dates_container corresponding to two different sets of styles that we don't want to overlap.

When we convert our own packages to use withStyles, we could probably solve this by doing something like:

export default withStyles(() => ({
  DateRangePicker_container: {
    ...
  },

  DateRangePicker_container_vertical: {
    ...
  },

  ...
}))(DateRangePicker));

to keep them unique, but it'd be nice if there was an easy way to convert existing packages or even our own for use with the CSS interface. The ideal of course is if we can just use the component name but again, that gets stripped out at runtime when using something like babel (which is common). If we could just pass in a name for each set of styles that can be ignored by most interfaces, that seems like the best case scenario, no? e.g.

export default withStyles(() => ({
  container: {
    ...
  },

  container_vertical: {
    ...
  },

  ...
}), 'DateRangePicker')(DateRangePicker));

[ljharb]

The reason that would be bad is that we'd end up with a subset of withStyles components that worked with some interfaces, and a different subset that worked with others.

The explicit goal of withStyles is that 100% of components work with 100% of interfaces, without modifications to any components.

[majapw]

@fvgs @ljharb @lencioni So I talked a bit more with @ljharb about this, and the conclusion that we came to is that it is probably unnecessary to add this functionality.

In the CSS interface, we should prepend a component's displayName if it exists using https://www.npmjs.com/package/function.prototype.name (if the style already follows the ${displayName}__style convention we can ignore it) and do nothing otherwise. We can add a comment both in the code and in the README that warns users that if displayName/name is unavailable that they may run into conflicts with css classes.

The primary reason that we can't always rely on component name seems to be the uglify babel plugin. Fortunately, the plugin has an option (--no-mangle-functions) to preserve names for our use. We should tackle this problem from two directions. 1) We should encourage people to use this config in our readme for truly unique names and 2) we should write our own public withStyles calls to have unique styles names regardless of whether or not name/displayName exist.

Thoughts?

[jeremy-clearlabs]

@majapw Was there anything added to the Readme regarding an issue around the uglify babel plugin for the component name?

I've followed the direction on your comment to add an option to add --no-mangle-functions, which if you're using the webpack uglifier, is under keep_fnames. (After of course banging my head around it). I can add a PR for adding this.

[ljharb]

@jeremy-clearlabs a PR to add that note would be very appreciated!