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

Distribute @apollo/server-integration-testsuite as CJS (correctly) #7055

Merged
merged 4 commits into from
Oct 18, 2022

Conversation

trevor-scheer
Copy link
Member

Right now the integration testsuite package declares "type": "module" but builds as CJS. This seems to be problematic for some integrations, but we should resolve the misconfiguration regardless.

This also moves the tsconfig/build "things" over to the CJS branch of our build step.

Confirmed this doesn't cause a regression in our local dev in that a change in test code is immediately reflected in a test run without running an additional build.

Fixes #7042

@netlify
Copy link

netlify bot commented Oct 18, 2022

Deploy Preview for apollo-server-docs ready!

Name Link
🔨 Latest commit e646523
🔍 Latest deploy log https://app.netlify.com/sites/apollo-server-docs/deploys/634ef50c7bc2bb00080acc6f
😎 Deploy Preview https://deploy-preview-7055--apollo-server-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 18, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e646523:

Sandbox Source
Apollo Server Typescript Configuration
Apollo Server Configuration
Apollo Server Issue #7042

@trevor-scheer
Copy link
Member Author

trevor-scheer commented Oct 18, 2022

@tobiasdiez I've tested the build (produced by codesandbox: https://ci.codesandbox.io/status/apollographql/apollo-server/pr/7055/builds/306007) in the h3 repo on this PR:
apollo-server-integrations/apollo-server-integration-h3#14

When running vitest via pnpm test it looks like we're uncovering a new error, which we haven't encountered in our usages:

Do not import `@jest/globals` outside of the Jest test environment

@SimenB it seems like our published testsuite is violating Jest expectations of usage (or vitest is exhibiting unexpected behavior in the h3 case in the PR above). Is there a correct way or pattern to go about exporting and publishing a testsuite that we might be missing?

@trevor-scheer
Copy link
Member Author

@tobiasdiez reports that he doesn't expect this to work in vitest and I've confirmed that this does indeed fix their jest usage via the test:integration script in the h3 repo.
apollo-server-integrations/apollo-server-integration-h3#14 (comment)

I am still curious to know about whether we're going about this package the right way at all. Things seem to be working, but that @jest/globals error leads me to believe that we shouldn't really be doing this and it might just be working but unintended use / prone to breakage in the future.

@SimenB
Copy link
Contributor

SimenB commented Oct 18, 2022

@jest/globals only works within jest - it's a "fake" module which throws the error you see above if loaded in other environments. As long as whatever you publish is only loaded in tests that run with Jest, there shouldn't be an issue. But e.g. bundling it (if that's what you're doing?) won't work either (i.e. the require/import must run via jest-runtime (or whatever vitest has if there's interop) with its original specifier: https://github.com/facebook/jest/blob/90995dfddbd51d6885d646dece3d37a1e3ad7fef/packages/jest-runtime/src/index.ts#L534-L544, https://github.com/facebook/jest/blob/90995dfddbd51d6885d646dece3d37a1e3ad7fef/packages/jest-runtime/src/index.ts#L534-L544).


I have zero idea how vitest works or how it works/interops with Jest - I've never used it or even looked at it 😅

@trevor-scheer
Copy link
Member Author

@SimenB it's being compiled and distributed as a package for use within jest. Example usage here:
https://github.com/apollo-server-integrations/apollo-server-integration-koa/blob/main/src/__tests__/integration.test.ts#L7-L10

Sounds like we're just getting away with it.

@glasser
Copy link
Member

glasser commented Oct 18, 2022

OK, so it's OK for there to be requires or imports of @jest/globals in JS or DTS files that we publish in an npm package, as long as nobody tries to import our files in any context other than running jest?

@SimenB
Copy link
Contributor

SimenB commented Oct 18, 2022

Yeah, as long as it's still a require('@jest/globals') or import '@jest/globals' (or import('@jest/globals')) when the test runner is running, that should work perfectly fine 🙂 If it doesn't, I'd consider that a bug

tsconfig.test.json Outdated Show resolved Hide resolved
@glasser
Copy link
Member

glasser commented Oct 18, 2022

The PR looks good to my limited understanding of the issues. My one bit of confusion is — if we're only going to build this major part of our test suite as CJS, maybe we shouldn't be trying to run our tests as ESM? jest.config.base.js has extensionsToTreatAsEsm: ['.ts'] and passes useESM: true to ts-jest. I've kinda suspected that maybe we are configuring ts-jest incorrectly or something, but should we be more consistent here?

@SimenB
Copy link
Contributor

SimenB commented Oct 18, 2022

OK, so it's OK for there to be requires or imports of @jest/globals in JS or DTS files that we publish in an npm package, as long as nobody tries to import our files in any context other than running jest?

d.ts is always safe FWIW - the error is only at runtime

Copy link
Member

@glasser glasser left a comment

Choose a reason for hiding this comment

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

we could consider adding some sort of smoke-test of the published package but that doesn't have to be in this PR.

@trevor-scheer trevor-scheer merged commit d0d8f4b into main Oct 18, 2022
@trevor-scheer trevor-scheer deleted the trevor/cjs-testsuite-fix branch October 18, 2022 19:18
trevor-scheer added a commit that referenced this pull request Oct 19, 2022
…7055)

Right now the integration testsuite package declares `"type": "module"`
but builds as CJS. This seems to be problematic for some integrations,
and we should resolve the misconfiguration.

This also moves the tsconfig/build "things" over to the CJS branch of
our build step. This also removes the ESM configuration options for how
we run jest/ts-jest in this repo.

Confirmed this doesn't cause a regression in our local dev in that a
change in test code is immediately reflected in a test run without
running an additional build.

Fixes #7042

<!--
First, 🌠 thank you 🌠 for taking the time to consider a contribution to
Apollo!

Here are some important details to follow:

* ⏰ Your time is important
To save your precious time, if the contribution you are making will take
more
than an hour, please make sure it has been discussed in an issue first.
          This is especially true for feature requests!
* 💡 Features
Feature requests can be created and discussed within a GitHub Issue. Be
sure to search for existing feature requests (and related issues!) prior
to
opening a new request. If an existing issue covers the need, please
upvote
that issue by using the 👍 emote, rather than opening a new issue.
* 🔌 Integrations
Apollo Server has many web-framework integrations including Express,
Koa,
Hapi and more. When adding a new feature, or fixing a bug, please take a
peak and see if other integrations are also affected. In most cases, the
fix can be applied to the other frameworks as well. Please note that,
since new web-frameworks have a high maintenance cost, pull-requests for
new web-frameworks should be discussed with a project maintainer first.
* 🕷 Bug fixes
These can be created and discussed in this repository. When fixing a
bug,
please _try_ to add a test which verifies the fix. If you cannot, you
should
still submit the PR but we may still ask you (and help you!) to create a
test.
* 📖 Contribution guidelines
Follow
https://github.com/apollographql/apollo-server/blob/main/CONTRIBUTING.md
when submitting a pull request. Make sure existing tests still pass, and
add
          tests for all new behavior.
* ✏️ Explain your pull request
Describe the big picture of your changes here to communicate to what
your
pull request is meant to accomplish. Provide 🔗 links 🔗 to associated
issues!

We hope you will find this to be a positive experience! Open source
contribution can be intimidating and we hope to alleviate that pain as
much as possible. Without following these guidelines, you may be missing
context that can help you succeed with your contribution, which is why
we encourage discussion first. Ultimately, there is no guarantee that we
will be able to merge your pull-request, but by following these
guidelines we can try to avoid disappointment.
-->
glasser pushed a commit that referenced this pull request Oct 21, 2022
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @apollo/server-integration-testsuite@4.0.2

### Patch Changes

- [#7035](#7035)
[`b3f400063`](b3f4000)
Thanks [@barryhagan](https://github.com/barryhagan)! - Errors resulting
from an attempt to use introspection when it is not enabled now have an
additional `validationErrorCode: 'INTROSPECTION_DISABLED'` extension;
this value is part of a new enum `ApolloServerValidationErrorCode`
exported from `@apollo/server/errors`.

- [#7066](#7066)
[`f11d55a83`](f11d55a)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Add a test
to validate error message and code for invalid operation names via GET

- [#7055](#7055)
[`d0d8f4be7`](d0d8f4b)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Fix build
configuration issue and align on CJS correctly

- Updated dependencies
\[[`b3f400063`](b3f4000)]:
    -   @apollo/server@4.0.2

## @apollo/server@4.0.2

### Patch Changes

- [#7035](#7035)
[`b3f400063`](b3f4000)
Thanks [@barryhagan](https://github.com/barryhagan)! - Errors resulting
from an attempt to use introspection when it is not enabled now have an
additional `validationErrorCode: 'INTROSPECTION_DISABLED'` extension;
this value is part of a new enum `ApolloServerValidationErrorCode`
exported from `@apollo/server/errors`.

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distribute integration-testsuite as esm
3 participants