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

feat: allow configuring identifiers in jest-transform plugin #841

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mihkeleidast
Copy link
Contributor

@mihkeleidast mihkeleidast commented Sep 22, 2022

Note: this is not working properly yet.

This adds an option do disable the unique identifiers in class names, similarly to how identity-obj-proxy could be used for CSS modules. This is useful in snapshot testing. Currently, there is a lot of noise in snapshot tests because of these identifiers - in my experience, they change quite often, as they take into account the order of the style rules. They also make the snapshots harder to read.

Usage in jest config:

transform: {
    '^.+\\.css.ts$': ['@vanilla-extract/jest-transform', { identifiers: () => '' }],
  },

Snapshot before:

exports[`Alert kind info 1`] = `
<div>
  <div
    class="alert_alert__qfrgk0 alert_kind_info__qfrgk4"
  >
    <svg
      class="icon_icon__15nmbj70 alert_icon__qfrgk5"
      focusable="false"
      height="16"
      width="16"
    >
      <use
        xlink:href="#common-ui-small-info"
      />
    </svg>
    <div
      class="alert_content__qfrgk7"
    >
      Alert body
    </div>
  </div>
</div>
`;

Snapshot after:

exports[`Alert kind info 1`] = `
<div>
  <div
    class="alert_alert alert_kind_info"
  >
    <svg
      class="icon_icon alert_icon"
      focusable="false"
      height="16"
      width="16"
    >
      <use
        xlink:href="#common-ui-small-info"
      />
    </svg>
    <div
      class="alert_content"
    >
      Alert body
    </div>
  </div>
</div>
`;

@changeset-bot
Copy link

changeset-bot bot commented Sep 22, 2022

🦋 Changeset detected

Latest commit: 4a741b7

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

This PR includes changesets to release 1 package
Name Type
@vanilla-extract/jest-transform Minor

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

const { identOption = 'debug' } = transformerConfig;
const { name: packageName } = getPackageInfo(config.rootDir);

setAdapter({
Copy link
Contributor Author

@mihkeleidast mihkeleidast Sep 22, 2022

Choose a reason for hiding this comment

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

This is part of the problem why I did not get it completely working. As it stands, in my Jest setup, this jest-transform setup loads the main-cjs file of the css/adapter submodule, but the actual @vanilla-extract/css imports load the browser bundle. This basically means that this transformer sets an adapter in one file, while the generateIdentifier function tries to read the value from another file.

Not sure if there is a better way to do what I'd like, but it seemed to be necessary to set an adapter that could be read from this config.

@mattcompiles
Copy link
Contributor

Hey @mihkeleidast 👋

This feature (or similar ideas) have been raised before. While I understand the reason to want it, I think it kinda goes against the fundamentals of Vanilla Extract. By adding this setting, local scoping is essentially disabled which is a lie and means we'll produce class names that can clash. I know this is only for targeting test environments but it could be used (and abused) in other environments and I also feel tests should match the real production code anyway.

Secondly, when we generate identifiers internally, the debug IDs are an optional parameter. I know in most cases you will have them due to the AST transformation, however I feel uncomfortable having a feature that depends on them being there.

Personally, I think having noisy HTML output goes hand-in-hand with using a CSS tool like Vanilla Extract. Maybe snapshotting HTML output isn't an ideal test?

@mihkeleidast
Copy link
Contributor Author

Maybe snapshotting HTML output isn't an ideal test?

Agree, but we're not snapshotting to get test coverage :)

Our typical test looks like this:

test("with label", () => {
  const { container } = render(<Button children="Button label" />);

  expect(screen.getByRole("button")).toHaveTextContent("Button label");
  expect(container).toMatchSnapshot();
  expect(container.innerHTML).toHTMLValidate();
});

Essentially, we're validating on the component level that the component itself produces valid HTML. The automated check, however, is not ideal, as a component with all divs and spans is also totally valid HTML. So we tend to also manually look at the snapshots to see if everything makes sense. The ability to manually read the snapshots also helps with pinpointing the issue when the automated checker fails with some error. When reading the snapshots, the actual underlying HTML is what matters, not the hashed class names returned by some tool. They "original names" do still help if wanting to align source code with output HTML, so I wouldn't want to use the import '@vanilla-extract/css/disableRuntimeStyles'; hook to disable them entirely.

Just some background, but it's fine if you still don't want to allow this, I understand the reasoning.

@mihkeleidast mihkeleidast changed the title feat: clean ident option feat: allow configuring identifiers in jest-transform plugin Aug 22, 2023
@mihkeleidast
Copy link
Contributor Author

@mattcompiles now that #1160 has landed, can we talk about configuring the identifiers in jest again? I cleaned the PR up to how it should be. Are you still opposed to allowing configuring this in jest?

However, I still cannot get it properly working like this due to jest loading different dist files for different parts of the VE ecosystem (cjs/esm/browser, etc). Would appreciate any pointers you may have as to how to get this actually working.

@askoufis
Copy link
Contributor

@mihkeleidast I don't see a reason not to support this in the jest transform.

I just ran into the same issue as you where the cjs jest transform is loaded but the other VE APIs are loading the browser version. I think this could be because the JSDom jest environment forces the browser export condition for all modules. This can be configured in jest, but that config is quite coarse and might break something else. I think the easiest way to fix this would be to add a browser export for the jest transform package.

However, I don't think this alone will fix the problem as we'll still need to set a custom adapter within the jest transform in order to set getIdentOption. I think you did this in your original implementation, but since you force-pushed last year I think that code is gone.

Would also be great to create a test case for this too if possible. Perhaps just a simple test inside the jest transform package. We could configure a transform pattern just for a specific file in that package and customize the identifiers function for it.

@mihkeleidast
Copy link
Contributor Author

Note that since I've recently switched jobs, I am not using vanilla-extract at the moment, so not very likely I can continue with this PR in the near future. If anyone else wants to take over, feel free to do so.

@askoufis
Copy link
Contributor

Note that since I've recently switched jobs, I am not using vanilla-extract at the moment, so not very likely I can continue with this PR in the near future. If anyone else wants to take over, feel free to do so.

No worries. I'm happy to keep working on this when I've get a chance. Thanks for getting the ball rolling.

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.

3 participants