createGenerateClassName: support option for ${Component}-${ruleName} in production for prod-like dev environment and integration testing

[rob2d]

Proposal Support a new property in createGenerateClassName options called fullRuleNamesInProd which simply omits short circuiting rule names if process.env.NODE_ENV == 'production'.

Reasoning/Background On some projects, it is preferred to use ComponentName-ruleName-ruleCounter in production for our integration tests via Puppeteer. This makes it much more versatile and uses a strength of JSS debugging to require much less work for selectors in integration tests. As an example, we might have a component FormContainer wrapping an action button called FormActionButton which represents re-usable async api caller buttons (e.g. login). In that case, a selector in a test for the button to be clicked might be written as

await page.$('div[class^="FormContainer"] div[class^="FormActionButton"]');

There's several benefits to this (enough to make a whole blog post if I was the motivated type):

1) reducing the need for an explicit data-id which just adds (unnecessary) overhead to development and being less D.R.Y. with changes, not to mention easier to target and read in inspector -- this is one of the primary reasons why in my personal use cases have used JSS on teams in recent projects. 2) the code is much more idiomatic and less to remember vs another layer of ids. 3) QA devs can quickly become familiar with the structure of the front-end components even if it's at a sort of superficial level vs blindly targeting ids. 4) there is a very minor perf hit, but the benefits above do not negate this enough. We still do get the benefit of smaller payloads as the rule names are generated on the fly and the name options property is not omitted in prod anyway once built, and cached rules.

In Material UI, there is a lot of boilerplate needed because of a simple check for process.env.NODE_ENV hardcoded in one place. I did see some discussion on this but didn't see any real questioning on why it was necessary. Its just a bit meh though to risk backwards compat code in the future to make this change since it already has support for full component/rule name in development (and this is also something a bit easier in JSS to accomplish without the abstraction of MUI typically -- but obviously we don't want to throw the baby out with the bathwater since this UI framework is GOAT).

Current Workaround I have created a utility locally in our application which encompassed material-ui's createGenerateClassName code passed to generateClassName which looks as following with only one line omitted which was the check for process.env.NODE_ENV and returns the prod-uglified/simplified behavior -- obviously this is not ideal as not all methods are exposed in Mui styles subspace, and it's just super not DRY or maintainable if things change subjecting us to some ugly things in prod.

[local-project-utils]/createGenerateClassName.js

// used to mimick dev classnames in prod

// ========================================================== //
// pulled from @material-ui/core/styles/ThemeProvider/nested

const hasSymbol = typeof Symbol === 'function' && Symbol.for;

const nested = (hasSymbol ? Symbol.for('mui.nested') : '__THEME_NESTED__');

// =========================================================== //

/**
 * This is the list of the style rule name we use as drop in replacement for the built-in
 * pseudo classes (:checked, :disabled, :focused, etc.).
 *
 * Why do they exist in the first place?
 * These classes are used at a specificity of 2.
 * It allows them to override previously definied styles as well as
 * being untouched by simple user overrides.
 */
const pseudoClasses = [
    'checked',
    'disabled',
    'error',
    'focused',
    'focusVisible',
    'required',
    'expanded',
    'selected',
];

export default function createGenerateClassName(options = {}) {
    const { disableGlobal = false, seed = '' } = options;
    const seedPrefix = (seed === '') ? '' : `${seed}-`;
    let ruleCounter = 0;

    return (rule, styleSheet) => {
        ruleCounter += 1;
        if(process.env.NODE_ENV !== 'production') {
            if(ruleCounter >= 1e10) {
                console.warn([
                    'Material-UI: you might have a memory leak.',
                    'The ruleCounter is not supposed to grow that much.',
                ].join(''));
            }
        }

        const { name } = styleSheet.options;

        // Is a global static MUI style?
        if(name && (name.indexOf('Mui') === 0) &&
            !styleSheet.options.link && !disableGlobal
        ) {

            // We can use a shorthand class name, we never use
            // the keys to style the components.

            if(pseudoClasses.indexOf(rule.key) !== -1) {
                return `Mui-${rule.key}`;
            }

            const prefix = `${seedPrefix}${name}-${rule.key}`;

            if(!styleSheet.options.theme[nested] || seed !== '') {
                return prefix;
            }

            return `${prefix}-${ruleCounter}`;
        }
        const suffix = `${rule.key}-${ruleCounter}`;

        // Help with debuggability.
        if(styleSheet.options.classNamePrefix) {
            return `${seedPrefix}${styleSheet.options.classNamePrefix}-${suffix}`;
        }

        return `${seedPrefix}${suffix}`;
    };
}

[client-root]/App.js

// before component
const generateClassName = createGenerateClassName();

// ... component and rendering
<StylesProvider generateClassName={ generateClassName }>
      <ThemeProvider theme={ themeApplied }>
          <App { ...appContent } />
     </ThemeProvider>
</StylesProvider>

Thanks for reading. Appreciate the great work on this library, and we are glad that things are flexible enough at least for a current workaround. At the same time, any consideration towards this usecase would be greatly appreciated here!

[side-note: if this sort of option was okay to consider by maintainers, I am fussy at all on the naming convention; simply proposed the above so I could try to provide a meaningful P.R. as a starting point]

Edits: for clarity and consistency/typos... and trying to be a bit more objective with the language used -- was in a bit of a rush to fit this in today.

[oliviertassinari]

it is preferred to use ComponentName-ruleName-ruleCounter in production for our integration tests via Puppeteer.

@rob2d we would highly discourage the usage of random (ruleCounter) generated class names to run end-to-end tests.

Instead, I would propose that we make the Mui prefix for global class name generation configurable. I have already seen a couple of developers ask for this option. This would allow you to manually set the name of your style sheets. Then, it becomes your responsibility to guarantee the uniqueness and avoid collisions. Would it work for you?

[rob2d]

Hi @oliviertassinari,

Thanks a lot for actually putting the time to consider a fix for the issue! Apologies for the delay here; I agree: randomized collisions/names are not a good idea, and that example was very off-the-top of my head since I didn't want to use work code directly + was just trying to make time between lots of things to PR. Its actually not an issue though as we go with [*="Component-rule-"] in most cases -- the dash itself is not a natural character in JSS namespaces or JSON keys (obviously it can be, but this is not normal/good practice), so it's also a natural delimeter.

Unfortunately, the suggestion you provided would not really solve the fundamental workflow of dev + prod in sync without a lot of extra steps/deliberation for testing (which in "agile" envs, is very difficult to prioritize IRL). We're also trying to leverage JSS itself for its strengths in our choices (e.g. can debug easily with minimal overhead to generation of rules + cache-ing, etc, aside from the typical React things with class over style that I know you're fully aware of by the nature of how great the library has matured).

I am not one to worship a document or something, but a good overview which I'm sure you've heard of which can address these points of workflow and having one cohesive way of doing things which explains much better than I can is via twelve factor apps, and can be found here: https://12factor.net/dev-prod-parity

Because of the gap between dev/prod, writing integration tests currently must be an afterthought without a workaround -- workflow and keeping things lean including the number of steps/abstraction to write tests in code is pretty important on some teams; even if it's as superficial as targeting things based on components/rule names as we write code. If you're not a huge TDD fan like myself and want some level of creativity without overhead, having a workflow which requires memorizing less and doing less, or changing less is very important to keeping the habit of testing.

On the cultural aspect of this sort of issue of uglifying classes automatically -- even putting the fact that there are optimizations -- unless one is in a place to worry about data scraping/reverse engineering... which hey, might be a relevant concern depending on your business -- even top notch tech companies who need to reach companies on 3G using CSS rely on things such as BEM conventions on their production websites despite the extra payload which is not an issue in JSS; see any page on:

http://www.squarespace.com http://www.apple.com http://www.wordpress.com https://github.com/ https://slack.com

and probably 30-40% of other websites as a reference on this.

I am aware of the blog article which has inspired the approach you guys take as thoughtfully mentioned by one of your PR approvers -- https://kentcdodds.com/blog/making-your-ui-tests-resilient-to-change -- knew that going in, and especially writing tests in other environments and before CSSinJS. However I feel that for some users use case it is not always relevant -- it was written with several ideas in mind which make it not entirely applicable to react-jss or CSSinJS in general, which doesn't say these concepts are negative or bad -- on the contrary, having a philosophy with certain things and not questioning makes some things very sane (e.g. linters). But they not always applicable in some teams and should be made as a conscious decision since:

1. CSS is not provided as static .css files anymore. There is no payload savings when uglifying, as there is in traditional CSS (which many views are reflective of to today and there's some pretty dogmatic stuff on that, for better or worse).

2. there is very minimal overhead to provide jss${ruleCount} vs ${sheetName}${ruleName}${ruleCount}, because:

   • (a) strings are pooled even in chunks in JS internally in Webkit/Chrome's v8 engine and other modern browsers when using concat and then runs through string interning when there's specific delimeters -- in addition the rules are not evaluated constantly but only on reflow for specific components using useEffect internally in JSS. For some refs: https://stackoverflow.com/questions/5276915/do-common-javascript-implementations-use-string-interning

• (b) because of the above, the repetitive nature of Component names is not as expensive by a long shot as one would seem when taking this into account based on older browsers when many practices were adopted.
• (c) while there is, it is imperceptible on a well written client app even on older machines to the point where 60fps is not a primary concern (and even in Africa, we're approaching 3g/4g capable mobile devices very quickly... which means we're talking GHz and not MHz: https://www.opensignal.com/2019/04/11/south-africas-smartphone-users-experience-faster-download-speeds-on-mobile-than-wifi)

Below is a sample of a workflow for arbitrary/common app scenario (writing a UI form), with some annotations. I realize your daily workflow might be different, so this is simply to illustrate if that's the case since I honestly have no idea and people I work with have found it sane/very intuitive.

In this way, without adding a single micro-step outside of styling/writing code, any level dev can begin writing tests simply from querying right in the window based on the work we were already doing. Since QA devs tend to also be regular front end engineers regularly and especially if training new or junior members, there is a lot of benefit to not needing to manually inject a data-test-id attribute:

• it encourages learning CSS selectors properly.
• it encourages using intuitive classnames and structures for your own components written in JSS, so that you do not have to spend so much time switching mental context and mimicking everything on UI phrases perfectly.
• if you did have a QA or junior developer, there is a lot less noise in the codebase itself. It is self documenting and they do not need to wade through random meaningless DOM attributes since test ids are not automated based on label content (which hey 💡 possible future idea?).

If able to just write markup, then code, without pausing to write another abstraction for tests which may differentiate from UI/UX e.g. labels and YAT maintainance overhead, then productivity goes up as procrastination goes down since we can get in the flow as devs. Then end of day, CI/CD does it's job anyway, and integration tests are less of an afterthought once frameworks/env were set up to work with.

As the creator of this lib, and with explanation of the issue out of the way: I do have to again say thanks a lot for your consistent work on this really nice library and for taking the time to respond here without shooting down the idea -- as well as always being patient with requests. If you don't feel it's an appropriate suggestion, it is completely cool. Mostly took the time here to try and be thorough as the response I got on PR meant there's obviously something I haven't communicated properly, and maybe that would make it easier at least to get a sense of where we're coming from.

For our issue, either way, I can just personally go with the workaround. But one thing which would be great (and also, this is not necessary. The consideration is more than enough as I know there are many priorities): if smaller pieces (hasSymbol, pseudoClasses) were exposed somewhere so there aren't multiple moving pieces when trying to mimick the ${ComponentName}-${ruleClass}-${ruleCount} issue -- similar to creating custom breakpoints today. I guess at the very least I would just hope the ability to do what my team is doing isn't deprecated so our 4.x apps don't become legacy shortly 🙏

So with this being a super lengthly response and all, TLDR:

• unfortunately, the proposed solution doesn't quite fit our specific wants/needs
• no real need for including rule numbers with query selectors via [*=X-y-] pattern.
• the recommendations for testing generally accepted are a sane approach but does not cover context of some modern tooling like CSSinJS.
• having meaningful classes across envs makes tests less annoying when refactoring to match UI/UX based on test ids, and those are ironically not user friendly when up-speeding devs as simply learning what components and rules are.
• we can/will continue workaround if the benefits of flexibility w/approach don't come off as warranted in your op.
• despite some interesting experiences lately, excellent library overall, and I've been more or less an advocate since 1.x.

Thanks! 🙃 🍻

[oliviertassinari]

We appreciate the time and the care put into this concern, however, I doubt any member of the team will have the time to read the whole content of the message.

From my perspective, this is a low priority issue. 1. The core components already generate global class names, we will keep it this way. 2. We plan to move to styled-components, emotion, and react-jss, interrupting @material-ui/styles effort. 3. Jss would be a better repository to push the concern further. 4. In the past, I had a use case for generating global class names at the application level (for heap analytics) we solved it by providing the name information to all the components and customizing the class name generator.

[eps1lon]

the recommendations for testing generally accepted are a sane approach but does not cover context of some modern tooling like CSSinJS.

Could you explain this in detail on the testing-library spectrum? testing-library is built to ignore implementation details such as the styling solution. We're using it and do use a css-in-js library. Would be interested to here what kind of components are not testable with testing-library

[rob2d]

@oliviertassinari thanks for the heads up on that. That sounds like a great move and probably could free up a lot of your time.

[rob2d]

the recommendations for testing generally accepted are a sane approach but does not cover context of some modern tooling like CSSinJS.

Could you explain this in detail on the testing-library spectrum? testing-library is built to ignore implementation details such as the styling solution. We're using it and do use a css-in-js library. Would be interested to here what kind of components are not testable with testing-library

Deleted my reply as I misread something fundamental there. But no issues at all with the lib you suggested. It was just matter of the feature request/flexibility. In any case, thanks a lot for the time and sorry for the bother. Keep up the good work on the lib guys 👍👍