Comments (18)
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?
from react-with-styles.
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.
from react-with-styles.
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.
from react-with-styles.
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.
from react-with-styles.
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?
from react-with-styles.
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.
from react-with-styles.
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).
from react-with-styles.
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?
from react-with-styles.
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.
from react-with-styles.
@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.
from react-with-styles.
True. However, that seems like a reasonable requirement imo for debugging anyways.
from react-with-styles.
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.
from react-with-styles.
Yes, component names could then be attached to stack traces, and show up in dev tools.
from react-with-styles.
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));
from react-with-styles.
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.
from react-with-styles.
@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?
from react-with-styles.
@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.
from react-with-styles.
@jeremy-clearlabs a PR to add that note would be very appreciated!
from react-with-styles.
Related Issues (20)
- An in-range update of enzyme is breaking the build 🚨 HOT 8
- How can I access child functions? HOT 1
- An in-range update of babel-preset-airbnb is breaking the build 🚨 HOT 2
- How to write hover effect HOT 1
- Contextualized theme ? HOT 1
- An in-range update of enzyme is breaking the build 🚨 HOT 1
- How can I return a react component using withStyles? HOT 2
- An in-range update of eslint-plugin-react is breaking the build 🚨 HOT 2
- Provide a `useStyles` method for using with React Hooks API HOT 5
- An in-range update of eslint-plugin-import is breaking the build 🚨 HOT 2
- An in-range update of eslint-plugin-jsx-a11y is breaking the build 🚨 HOT 2
- .git included in npm distribution in version 3.2.2 HOT 4
- how to get styles object into class type component that extends React.component HOT 5
- An in-range update of eslint-plugin-import is breaking the build 🚨 HOT 3
- Uncaught TypeError: Super expression must either be null or a function HOT 1
- An in-range update of eslint-plugin-react is breaking the build 🚨 HOT 2
- Selectors in CSS/JS not working as expected
- Context provider does not provide the interface HOT 1
- Add testing on React 17, once Enzyme has an adapter for it
- how to access children styles HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from react-with-styles.