Skip to content
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

Merged
merged 5 commits into from
Aug 21, 2023

Conversation

SombreroElGringo
Copy link
Contributor

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,

@changeset-bot
Copy link

changeset-bot bot commented Aug 10, 2023

🦋 Changeset detected

Latest commit: 6e3751c

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

This PR includes changesets to release 14 packages
Name Type
@vanilla-extract/css Minor
@vanilla-extract/webpack-plugin Minor
@vanilla-extract/esbuild-plugin Minor
@vanilla-extract/rollup-plugin Minor
@vanilla-extract/vite-plugin Minor
@vanilla-extract/next-plugin Minor
@fixtures/features Patch
@fixtures/layers Patch
@fixtures/low-level Patch
@fixtures/recipes Patch
@fixtures/sprinkles Patch
@fixtures/themed Patch
@fixtures/unused-modules Patch
vanilla-extract-example-webpack-react 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

Copy link
Contributor

@mattcompiles mattcompiles left a 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.

type IdentOption =
| 'short'
| 'debug'
| ((scope: string, refCount: string, debugId: string | undefined) => string);
Copy link
Contributor

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 be scope + refCount. Let's just call it hash.
  • Maybe also add filename and packageName to be fed into a custom hash

Copy link
Contributor Author

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.

@@ -38,6 +42,7 @@ export function generateIdentifier(options?: GenerateIdentifierOptions): string;
export function generateIdentifier(
arg?: string | GenerateIdentifierOptions,
): string {
const indentOption = getIdentOption();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const indentOption = getIdentOption();
const identOption = getIdentOption();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -0,0 +1,6 @@
---
'@vanilla-extract/css': minor
'site': minor
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

@mattcompiles mattcompiles left a 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.

type CustomIdentFunction = (params: {
hash: string;
debugId?: string;
filePath?: string;
Copy link
Contributor

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

Suggested change
filePath?: string;
filePath: string;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

site/docs/integrations/esbuild.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants