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(snapshot): support snapshotResolver and snapshotSerializers written in ESM #12014

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chentsulin
Copy link
Contributor

@chentsulin chentsulin commented Oct 29, 2021

Summary

Part of #11167.

See the discussion on the snapshotResolver and snapshotSerializers config: #11167 (comment) #11167 (comment)

Test plan

Integration test added.

@chentsulin chentsulin force-pushed the import-snapshotResolver-snapshotSerializers branch from f8aa171 to 2cab732 Compare October 30, 2021 03:56
it('defining tests and hooks asynchronously throws', () => {
const result = runJest('circus-declaration-errors', [
it('defining tests and hooks asynchronously throws', async () => {
const result = await runJest('circus-declaration-errors', [
Copy link
Contributor Author

@chentsulin chentsulin Oct 30, 2021

Choose a reason for hiding this comment

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

This runJest function actually doesn't return a promise, so we should find a way to test this kind of stuff.
This test got failed after I put runtime.requireModule into a async function.

From:

const esm = runtime.unstable_shouldLoadAsEsm(testPath);

if (esm) {
  await runtime.unstable_importModule(testPath);
} else {
    runtime.requireModule(testPath);
}

To:

const localRequire = async <T = unknown>(path: string): Promise<T> => {
  const esm = runtime.unstable_shouldLoadAsEsm(path);

  if (esm) {
    return runtime.unstable_importModule(path) as any;
  } else {
    return runtime.requireModule(path);
  }
};

await localRequire(testPath);

Copy link
Member

Choose a reason for hiding this comment

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

runJest spawns jest in a shell, so it should not be affected by any internal changes in Jest

@chentsulin chentsulin force-pushed the import-snapshotResolver-snapshotSerializers branch 4 times, most recently from 933e7ca to b961505 Compare October 30, 2021 05:29
@chentsulin chentsulin marked this pull request as ready for review October 30, 2021 05:30
@chentsulin chentsulin force-pushed the import-snapshotResolver-snapshotSerializers branch 2 times, most recently from d19c3af to ddb4eeb Compare October 30, 2021 08:07
const evaluatedModule = await this.linkAndEvaluateModule(module);

return evaluatedModule instanceof SourceTextModule
? evaluatedModule.namespace.default
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no clue what to do here, but I found this method sometimes returns vm.SourceTextModule as result.

Copy link
Member

Choose a reason for hiding this comment

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

There is no "result" of executing a module. Do we need access to evaluated module? We haven't needed that before... We might need an unstable_importInternalModule which essentially does what you do here (wait for evaluatedModule.status === 'evaluated', then fetch its default export).

Essentially faking an import() call

Copy link
Contributor Author

@chentsulin chentsulin Nov 3, 2021

Choose a reason for hiding this comment

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

If i didn't get it wrong, the result is used by this localRequire call to support ESM resolvers:

const custom: SnapshotResolver = await localRequire(
  snapshotResolverPath,
  true,
);

).default;
const custom: SnapshotResolver = await localRequire(
snapshotResolverPath,
true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a boolean parameter to localRequire to support interopRequireDefault here.

@chentsulin chentsulin force-pushed the import-snapshotResolver-snapshotSerializers branch 2 times, most recently from d65360d to d8033b6 Compare October 30, 2021 14:18
@Maxim-Mazurok
Copy link
Contributor

@chentsulin I wonder if this could be finalized or if it is waiting for a re-review from @SimenB ?...

We need this here: https://github.com/Maxim-Mazurok/google-api-typings-generator/blob/062854a2021be2dd244855261a73f5172bf597c2/custom-resolver.cjs for the use-case of using one snapshot file per test.

Thank you!

@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

Main issue is that we load the resolver within the tests (#12014 (comment)), which means it's pretty much blocked by full ESM support

@Maxim-Mazurok
Copy link
Contributor

Maxim-Mazurok commented Sep 28, 2022

Ah, I see, thank you for the update, we are re-considering our approach now. Cheers!

@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

It might make sense to not load it within, but that means people who have written it in TS or something that requires transpilation will fail. So not sure which tradeoff is the best here

@Anutrix
Copy link

Anutrix commented Sep 21, 2023

It's been a year. Any updates?

Copy link

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Sep 21, 2024
@Anutrix
Copy link

Anutrix commented Sep 22, 2024

It's been another year. Any updates?

@github-actions github-actions bot removed the Stale label Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants