Ensure sc- prefix is always added

[chalbert]

Currently, the sc- is added at the start of the hash by replacing the first digit. The problem is that the hash may also start by a letter, causing the class name to sometimes miss the sc- prefix.

This fix makes it possible to use this plugin with styled-components@5 + jest-styled-components@7.

In the meantime, I've published the fix on npm under @chalbert/babel-plugin-styled-components

In my experience, it fixes the following issues :

#268
styled-components/jest-styled-components#297
styled-components/jest-styled-components#335
styled-components/jest-styled-components#294
And potentially others.

[quantizor]

The displayName shouldn't be getting prefixed

[chalbert]

Right, I see. That may be why the bug was first introduced. By only prefixing digit, the goal was to skip prefixing the displayName, but it also skipped prefixing some of the hashes. I'll look into it.

[chalbert]

@probablyup I've updated to prevent the prefixing of the displayName.

[agriffis]

What about something like this instead?

• generated classes consistently get sc- prefix
• block names get an underscore prefix when they would otherwise be invalid

diff --git a/src/utils/prefixDigit.js b/src/utils/prefixDigit.js
index 8cb8c4b..ec5d2ee 100644
--- a/src/utils/prefixDigit.js
+++ b/src/utils/prefixDigit.js
@@ -1,3 +1,3 @@
-export default function prefixLeadingDigit(str) {
-  return str.replace(/^(\d)/, 'sc-$1')
+export default function prefixLeadingDigit(str, prefix) {
+  return str.replace(/^(?=\d)/, prefix)
 }
diff --git a/src/visitors/displayNameAndId.js b/src/visitors/displayNameAndId.js
index 42c1c1f..eed1140 100644
--- a/src/visitors/displayNameAndId.js
+++ b/src/visitors/displayNameAndId.js
@@ -72,8 +72,8 @@ const getDisplayName = t => (path, state) => {
       return componentName
     }
     return componentName
-      ? `${prefixLeadingDigit(blockName)}__${componentName}`
-      : prefixLeadingDigit(blockName)
+      ? `${prefixLeadingDigit(blockName, '_')}__${componentName}`
+      : prefixLeadingDigit(blockName, '_')
   } else {
     return componentName
   }
@@ -134,10 +134,8 @@ const getNextId = state => {
 }
 
 const getComponentId = state => {
-  // Prefix the identifier with a character because CSS classes cannot start with a number
-  return `${useNamespace(state)}${prefixLeadingDigit(
-    getFileHash(state)
-  )}-${getNextId(state)}`
+  // Ensure identifier has prefix to avoid invalid class starting with number.
+  return `${useNamespace(state)}sc-${getFileHash(state)}-${getNextId(state)}`
 }
 
 export default t => (path, state) => {

[chalbert]

With this different approach, when using a namespace, the classname does not include sc- anymore. I don't know if there is a reason for that. It could be a breaking change for tools or projects that rely on having the sc- in the classname.

[agriffis]

With this different approach, when using a namespace, the classname does not include sc- anymore. I don't know if there is a reason for that. It could be a breaking change for tools or projects that rely on having the sc- in the classname.

It wasn't consistent previously, because of conditionalizing on the digit. I doubt it's a problem.

If you feel strongly about it, that line could be ${useNamespace(state)}sc-${getFileHash(state)}-${getNextId(state)} to always insert sc- regardless of the namespace, but my feeling is that the namespace serves the purpose at that point, and the extra sc- is just noise.

[chalbert]

Not being a maintainer, I'm not too sure of the impact. What I know is that jest-styled-components uses sc- to locate the class ― it was at the root of this bug as it was sometimes missing. So I guess it would be safer to keep sc- even if it does impose a bit of noise.

[agriffis]

Sure, that makes sense. I just updated my diff above.

Main thing I'd like to avoid is extra complexity. There are two kinds of prefixing here: (1) for eyeballing and tools, (2) to avoid invalid class names. These different reasons were conflated by the original code. Rather than make that code more complex to cover more cases, it's better to tease them apart.

If you like it, please feel free to update your PR to use this approach.

[chalbert]

@probablyup I simplified the solution.

[akatakritos]

Thanks @chalbert -- I've been troubleshooting this all morning after an upgrade. Your forked package is working great for me now.

Would love to get this merged in!

[quantizor]

@chalbert could you merge from main to get the tests running? Just enabled actions for forks. TY!

[quantizor]

Makes sense