Add the option to provide a prefix to generated class names #1717

guilhermehto · 2024-10-09T03:25:48Z

What is this change?

This PR introduces a new configuration prop to @compiled/babel-plugin named classHashPrefix (feel free to suggest a better alternative) that allows users to define a prefix to be added to when generating class name hashes.

Why are we making this change?

We have an internal problem in a consumer of a remote module where it has its styles messed up due to the loss of specificity once the remote module is loaded. This is because both the wrapping application and the remote module use @compiled and due to the way we generate the class names, both end up generating a few with the same name.

How are we making this change?

This is the first time I ever work with a Babel plugin so I might be missing something, all feedback is appreciated. I tried to follow the code and tests to figure out where we were generating the hashes and hooked it up with the new config prop.

Lastly, I only worked off of tests on this, guidance on how to better test it are welcomed 😅

edit: I assume I'm probably missing a changeset on this pr done

PR checklist

Don't delete me!

I have...

Updated or added applicable tests
Updated the documentation in website/

changeset-bot · 2024-10-09T03:25:51Z

🦋 Changeset detected

Latest commit: 7ad0484

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@compiled/parcel-transformer	Minor
@compiled/webpack-loader	Minor
@compiled/babel-plugin	Minor
@compiled/css	Minor
@compiled/parcel-config	Patch
@compiled/babel-plugin-strip-runtime	Minor
@compiled/parcel-optimizer	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2024-10-09T03:26:00Z

✅ Deploy Preview for compiled-css-in-js ready!

Name	Link
🔨 Latest commit	`7ad0484`
🔍 Latest deploy log	https://app.netlify.com/sites/compiled-css-in-js/deploys/670c360344a3f50008ef0edd
😎 Deploy Preview	https://deploy-preview-1717--compiled-css-in-js.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

JakeLane

How does this work with ax? That parses the classname and I don't think it'll work without changes.

Have a read of https://github.com/atlassian-labs/compiled/blob/master/packages/react/src/runtime/ax.ts#L30 and probably add some tests there

JakeLane · 2024-10-09T04:14:51Z

packages/babel-plugin/src/__tests__/index.test.ts

@@ -205,6 +205,36 @@ describe('babel plugin', () => {
    expect(actual).toInclude('c_MyDiv');
  });

+  it.only('should add a prefix to style hash hashPrefix is present', () => {


Suggested change

it.only('should add a prefix to style hash hashPrefix is present', () => {

it('should add a prefix to style hash hashPrefix is present', () => {

guilhermehto · 2024-10-09T22:57:15Z

hey @JakeLane I was playing around with the code but I couldn't find any clear solutions besides adding the hash prefix into the hash of the group. This way groups would wind up with the same hash and ax should work as expected, I believe 🤔

Essentially

const group = hash(`${opts.atRule}${selectors}${node.prop}`).slice(0, 4);
// becomes
const group = hash(`${opts.hashPrefix}${opts.atRule}${selectors}${node.prop}`).slice(0, 4);

Seems too easy to be true though, am I potentially missing something?

edit: Obviously we'd lose the ability to check for the prefix when looking through the class names as they'd be hashed but that's not really why we're making this change to begin with so I think it's an acceptable tradeoff.

pancaspe87 · 2024-10-10T05:06:15Z

packages/babel-plugin/src/types.ts

+   * Adds a defined prefix to the generated classes' hashes.
+   * Useful in micro frontend environments to avoid clashing/specificity issues.
+   */
+  hashPrefix?: string;


nit

what about addClassPrefix? cc @dddlr @JakeLane

yeah more descriptive naming is helpful

Imo add suggests the expected value is a boolean and not a string that we're setting. Would classHashPrefix be better in this case?

ye that's fine

JakeLane · 2024-10-10T06:19:56Z

hey @JakeLane I was playing around with the code but I couldn't find any clear solutions besides adding the hash prefix into the hash of the group. This way groups would wind up with the same hash and ax should work as expected, I believe 🤔

Essentially
const group = hash(`${opts.atRule}${selectors}${node.prop}`).slice(0, 4);
// becomes
const group = hash(`${opts.hashPrefix}${opts.atRule}${selectors}${node.prop}`).slice(0, 4);
Seems too easy to be true though, am I potentially missing something?

edit: Obviously we'd lose the ability to check for the prefix when looking through the class names as they'd be hashed but that's not really why we're making this change to begin with so I think it's an acceptable tradeoff.

I think this is fine. It might be good to validate VR test correctness manually; I don't think we have a way to change the babel config on VR tests right now.

dddlr · 2024-10-10T23:39:07Z

website/packages/docs/src/pages/pkg-babel-plugin.mdx

@@ -93,6 +94,15 @@ Extensions that we should consider code. We use these to identify if a file shou
 - Type: `string[]`
 - Default: `['.js', '.jsx', '.ts', '.tsx']`

+#### hashPrefix
+
+Adds a prefix to the generated hashed css rule names.


nit: mention what format hashPrefix is required to be

e.g. how it needs to match the /^[a-zA-Z\-][a-zA-Z\-_0-9]*$/ regex

ah yep good idea!

dddlr · 2024-10-10T23:46:12Z

packages/css/src/plugins/atomicify-rules.ts

 */
 const atomicClassName = (node: Declaration, opts: PluginOpts) => {
+  if (opts.hashPrefix && !isCssIdentifierValid(opts.hashPrefix)) {


nit: what if we move this check to the atomicifyRules function later in the file? we don't expect opts.hashPrefix to change during the build stage right

(not a strong preference, i don't mind either way)

can do! Probably a better idea, we most likely don't wanna be running regex matches all willy nilly

dddlr · 2024-10-10T23:53:02Z

packages/css/src/plugins/atomicify-rules.ts

+ *
+ */
+const isCssIdentifierValid = (value: string): boolean => {
+  const validCssIdentifierRegex = /^[a-zA-Z\-][a-zA-Z\-_0-9]*$/;


where did you find this regex? it looks reasonable but it doesn't match with what i could easily find online e.g. https://stackoverflow.com/a/449000

I made it myself as it was supposed to be in the middle of an already existing identifier. It's actually not 100% now that I look at it;

That said, since it's now being used in the hash and not as a standalone string it could be pretty much anything but I think it makes sense to continue to only accept identifiers to prevent any potential unwanted side effects. I'll refactor it

dddlr · 2024-10-11T03:36:15Z

packages/css/src/plugins/atomicify-rules.ts

+ */
+const isCssIdentifierValid = (value: string): boolean => {
+  const validCssIdentifierRegex = /^[a-zA-Z\-_]+[a-zA-Z\-_0-9]*$/;
+  return Boolean(value.match(validCssIdentifierRegex));


oops sorry i just realised, can this be simplified to validCssIdentifierRegex.test(value) instead?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test

dddlr · 2024-10-11T03:36:37Z

packages/css/src/plugins/atomicify-rules.ts

@@ -21,10 +34,14 @@ interface PluginOpts {
 *
 * @param node CSS declaration
 * @param opts AtomicifyOpts
+ *
+ * @throws Throws an error if `opts.classHashPrefix` contains invalid css class/id characters


this @throws should be removed

… used with `ax`

liamqma · 2024-10-13T22:34:46Z

packages/babel-plugin/src/__tests__/index.test.ts

+      { classHashPrefix: 'myprefix' }
+    );
+
+    expect(actual).toInclude(hashedClassName);


~~should we check actual include classHashPrefix too?~~
All good. classHashPrefix is part of hash.

guilhermehto requested review from JakeLane, pancaspe87, liamqma and dddlr as code owners October 9, 2024 03:25

JakeLane requested changes Oct 9, 2024

View reviewed changes

pancaspe87 reviewed Oct 10, 2024

View reviewed changes

pancaspe87 requested a review from zesmith2 October 10, 2024 05:09

dddlr reviewed Oct 10, 2024

View reviewed changes

guilhermehto force-pushed the feat/add-hash-prefix branch from 5d9e370 to da5c433 Compare October 11, 2024 02:42

dddlr reviewed Oct 11, 2024

View reviewed changes

guilhermehto added 13 commits October 14, 2024 10:04

chore: extend types to support hashPrefix options

6531c06

feat: add prefix to generated hash rules

0a8e4e9

tests: add tests around hashPrefix

02150de

docs: add section around hashPrefix

95b5e5d

chore: add changeset

0f18c72

tests: remove .only from tests, whoops

296fad0

feat: add hashPrefix to the group name hash so it does not break when…

0241240

… used with `ax`

refactor: hashPrefix -> classHashPrefix

854f50b

feat: update validator regex and move validation to top level method

8657ee4

docs: update description for classHashPrefix

ec3ebf2

docs: sort

cd46315

chore: remove incorrect jsdoc

a3ac70e

refactor: use .test instead of .match

832ae2a

chore: update changeset description

7ad0484

guilhermehto force-pushed the feat/add-hash-prefix branch from 177098b to 7ad0484 Compare October 13, 2024 21:05

liamqma reviewed Oct 13, 2024

View reviewed changes

liamqma approved these changes Oct 13, 2024

View reviewed changes

dddlr approved these changes Oct 14, 2024

View reviewed changes

JakeLane approved these changes Oct 14, 2024

View reviewed changes

dddlr merged commit 4fb5c6e into master Oct 14, 2024
13 checks passed

dddlr deleted the feat/add-hash-prefix branch October 14, 2024 03:56

kylorhall-atlassian mentioned this pull request Oct 15, 2024

Throw an error when mixing extract: true and classHashPrefix configuration options to avoid unsupported usage and bundle size bloat. #1724

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the option to provide a prefix to generated class names #1717

Add the option to provide a prefix to generated class names #1717

guilhermehto commented Oct 9, 2024 •

edited

Loading

changeset-bot bot commented Oct 9, 2024 •

edited

Loading

netlify bot commented Oct 9, 2024 •

edited

Loading

JakeLane left a comment •

edited

Loading

JakeLane Oct 9, 2024

guilhermehto commented Oct 9, 2024 •

edited

Loading

pancaspe87 Oct 10, 2024

JakeLane Oct 10, 2024

guilhermehto Oct 10, 2024

dddlr Oct 10, 2024

JakeLane commented Oct 10, 2024

dddlr Oct 10, 2024

guilhermehto Oct 11, 2024

dddlr Oct 10, 2024 •

edited

Loading

guilhermehto Oct 11, 2024

dddlr Oct 10, 2024

guilhermehto Oct 11, 2024

dddlr Oct 11, 2024 •

edited

Loading

dddlr Oct 11, 2024

liamqma Oct 13, 2024 •

edited

Loading

	it.only('should add a prefix to style hash hashPrefix is present', () => {
	it('should add a prefix to style hash hashPrefix is present', () => {

Add the option to provide a prefix to generated class names #1717

Add the option to provide a prefix to generated class names #1717

Conversation

guilhermehto commented Oct 9, 2024 • edited Loading

What is this change?

Why are we making this change?

How are we making this change?

PR checklist

changeset-bot bot commented Oct 9, 2024 • edited Loading

🦋 Changeset detected

netlify bot commented Oct 9, 2024 • edited Loading

✅ Deploy Preview for compiled-css-in-js ready!

JakeLane left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhermehto commented Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeLane commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dddlr Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dddlr Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamqma Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

guilhermehto commented Oct 9, 2024 •

edited

Loading

changeset-bot bot commented Oct 9, 2024 •

edited

Loading

netlify bot commented Oct 9, 2024 •

edited

Loading

JakeLane left a comment •

edited

Loading

guilhermehto commented Oct 9, 2024 •

edited

Loading

dddlr Oct 10, 2024 •

edited

Loading

dddlr Oct 11, 2024 •

edited

Loading

liamqma Oct 13, 2024 •

edited

Loading