-
Notifications
You must be signed in to change notification settings - Fork 294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow custom identifier hashing #1160
Allow custom identifier hashing #1160
Conversation
🦋 Changeset detectedLatest commit: 6e3751c The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Thanks for the PR.
The microfrontend use-case convinces me this feature is necessary. I've left some comments to be addressed, but we can get this merged once that's done.
packages/css/src/types.ts
Outdated
type IdentOption = | ||
| 'short' | ||
| 'debug' | ||
| ((scope: string, refCount: string, debugId: string | undefined) => string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few thoughts about this:
- I think this should instead be an object for named params.
- I think
scope
is too abstract, seeing as this is a hash of the file path. I think it'd be more useful to provide a general hash for the identifier which would bescope + refCount
. Let's just call ithash
. - Maybe also add filename and packageName to be fed into a custom hash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for the suggestion, I updated its type and added the extra params suggested, also for the filename I think it makes sense to just pass the file path and give the freedom to the developer to parse this file path or X to retrieve himself the filename or do X operations on it.
packages/css/src/identifier.ts
Outdated
@@ -38,6 +42,7 @@ export function generateIdentifier(options?: GenerateIdentifierOptions): string; | |||
export function generateIdentifier( | |||
arg?: string | GenerateIdentifierOptions, | |||
): string { | |||
const indentOption = getIdentOption(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const indentOption = getIdentOption(); | |
const identOption = getIdentOption(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
.changeset/curly-lies-flash.md
Outdated
@@ -0,0 +1,6 @@ | |||
--- | |||
'@vanilla-extract/css': minor | |||
'site': minor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not version the site. Can we also add a minor for the affected plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with the affected packages, let me know if I missed some
…tion should accept an object as params - pass new params in custom ident function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, can you please update the type and the docs.
packages/css/src/types.ts
Outdated
type CustomIdentFunction = (params: { | ||
hash: string; | ||
debugId?: string; | ||
filePath?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filePath
will always be available
filePath?: string; | |
filePath: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
d784729
to
4add4a2
Compare
… - update CustomIdentFunction type
4add4a2
to
a7e6da8
Compare
Hello, I am submitting a request to reopen Pull Request #657. This proposed enhancement aims to unlock my team, which is currently facing a classname clash issue, we are using multiple micro-frontends within a host application. Unfortunately, some of the generated hash values from these micro-frontends coincide with hash values from the host application. We are really hoping for this feature to be integrated, like that it will allows us to customize the generated classnames by applying prefixes specific to each micro-frontend and will guarantee the uniqueness of every hash.
Moreover, the implementation of this feature could help multiple persons with similar scenarios and other challenges, we can found in the following discussion:
Also, I'd like to highlight a similar feature available in Material UI allowing to customize the generated classnames.
Thank you for your consideration.
Best regards,