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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature]: Restore Access to Utility Functions in "jest-snapshot/utils" for Continued Tooling Support #15036

Closed
connectdotz opened this issue Apr 24, 2024 · 10 comments 路 Fixed by #15095

Comments

@connectdotz
Copy link
Contributor

馃殌 Feature Proposal

Please consider re-exposing the following functions and corresponding types in jest-snapshot/utils.ts to enable tools like vscode-jest and jest-editor-support to retrieve snapshot content by test names effectively:

  • getSnapshotData
  • testNameToKey

In jest-snapshot version 27.x, these utilities were fully accessible at jest-snapshot/utils, facilitating robust tool integration. However, versions 28.x and 29.x have restricted access to these functions/types, significantly impacting tool functionality and integration.

Motivation

The primary motivation for this request is to maintain backward compatibility with existing features in community tools, such as the "view snapshot" feature in vscode-jest. This functionality, along with others, enables users to view, update, run, and debug snapshots for individual tests, is crucial for enhancing the user experience in VSCode's Jest integration.

Example

For instance, the jest-editor-support library uses functions like getSnapshotData and testNameToKey from jest-snapshot/utils to retrieve snapshot content. Here's how it's implemented in getSnapshotContent and getMetadata

Pitch

I understand the concerns about exposing unnecessary functions. However, considering their existing use within the tooling community, unless there is a better alternative, could we consider re-exposing these utility functions/types? If you agree with this proposal, I could submit a PR shortly.

@connectdotz
Copy link
Contributor Author

@SimenB do you object we re-expose these functions/types?

@mrazauskas
Copy link
Contributor

Perhaps the utils could simply be published as separate @jest/snapshot-utils package?

@connectdotz
Copy link
Contributor Author

I'm ok with either approach. @SimenB, what's your take?

@SimenB
Copy link
Member

SimenB commented May 12, 2024

Separate util package makes sense to me 馃憤 Happy to take a PR to that effect 馃檪

@connectdotz
Copy link
Contributor Author

@SimenB, a few follow-up questions regarding splitting the utils into its own package @jest/snapshot-utils:

Circular Reference

Is it correct to assume that you would prefer the @jest/snapshot-utils package not to depend on the @jest/snapshot package due to circular reference? If that is the case, we have the following dependency issues to resolve:

In utils.ts:

import {getSerializers} from './plugins';
import type {SnapshotData} from './types';
  • The ./plugins module is not used anywhere except in utils.ts. Should we move it to @jest/snapshot-utils as well?
  • The type SnapshotData from ./types is simply a Record<string, string>. Do we want to create a new package @jest/snapshot-types for a single line type definition? Or maybe the whole ./types file?

Re-evaluation

Given these issues, do we still favor splitting utils.ts into new packages (@jest/snapshot-utils, @jest/snapshot-types) over exposing it within @jest/snapshot?

@mrazauskas
Copy link
Contributor

mrazauskas commented May 18, 2024

In TypeScript repo they have public and internal utilities. Perhaps similar strategy could be adopted? I mean, the new package could exposing only getSnapshotData(), testNameToKey() and other actually useful utilities not all what utils.ts contains.

By the way, I think SnapshotData can be replaced with Record<string, string>. That looks fine, because TypeScript does not have nominal types.

UPDATE: Or SnapshotData could be exported from the new package, because it is part of the return type of getSnapshotData().

@SimenB
Copy link
Member

SimenB commented May 19, 2024

Moving SnapshotData to the new package should be fine, yeah

@connectdotz
Copy link
Contributor Author

sorry for the delay, I am back to this issue now. One more question, given the jest-snapshot doesn't use the "@jest" scope (not sure what the naming rule is), do we want the new package to be @jest/snapshot-utils or jest-snapshot-utils?

@SimenB
Copy link
Member

SimenB commented May 28, 2024

The old ones don't as they predate our ownership of the jest org on NPM. All new packages should use the namescaped one, but we don't wanna migrate old ones (just to avoid dependents getting left behind).

@SimenB
Copy link
Member

SimenB commented May 30, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants