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

chore: update jest-related dependencies to v28 #3400

Closed
wants to merge 3 commits into from

Conversation

penx
Copy link
Contributor

@penx penx commented Jun 6, 2022

Closes: Discussion at #3397 (comment)

  • Docs
  • Tests

Testing Strategy:

  • See if CI passes

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jun 6, 2022

Hi @penx,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jun 6, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@penx penx marked this pull request as draft June 6, 2022 15:00
@MichaelDeBoey MichaelDeBoey changed the title chore: update jest dependencies to v28 chore: update jest-related dependencies to v28 Jun 6, 2022
@penx
Copy link
Contributor Author

penx commented Jun 6, 2022

Moving to draft while I review the babel config changes https://jestjs.io/docs/upgrading-to-jest28#babel-config

@penx penx marked this pull request as ready for review June 6, 2022 15:16
@penx
Copy link
Contributor Author

penx commented Jun 6, 2022

Example failing test
FAIL react packages/remix-react/__tests__/transition-test.tsx
  ● Test suite failed to run

    Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/configuration
    For information about custom transformations, see:
    https://jestjs.io/docs/code-transformation

    Details:

    /home/runner/work/remix/remix/node_modules/@remix-run/web-fetch/src/lib.js:4
    export { ReadableStream, Blob, FormData  } from './package.js';
    ^^^^^^

    SyntaxError: Unexpected token 'export'

      30 |   NodeRequestInit as RequestInit,
      31 |   NodeResponseInit as ResponseInit,
    > 32 | };
         |   ^
      33 |
      34 | class NodeRequest extends WebRequest {
      35 |   constructor(info: NodeRequestInfo, init?: NodeRequestInit) {

      at Runtime.createScriptFromCode (../../node_modules/jest-runtime/build/index.js:1773:14)
      at Object.<anonymous> (../remix-node/fetch.ts:32:17)

I think the failing tests are due to the following from @remix-run/web-fetch:

  "exports": {
    ".": {
      "browser": "./src/lib.js",
      "require": "./dist/lib.node.cjs",
      "import": "./src/lib.node.js",
      "types": "./dist/src/lib.node.d.ts"
    },

https://github.com/remix-run/web-std-io/blob/f5a5e15c23247b335b8cd09d8728f957e482bc62/packages/fetch/package.json#L12

This means that Jest 28 tries to import src/lib.js which results in Unexpected token 'export'.

We can likely tell Jest to transpile this module but it should probably be fixed upstream.

@chaance ?

@penx
Copy link
Contributor Author

penx commented Jun 6, 2022

Further to this, I can add the following to the jest config:

  transformIgnorePatterns: [
    "/node_modules/(?!(@remix-run/web-fetch|@remix-run/web-blob|@remix-run/web-stream|@remix-run/web-form-data|@remix-run/web-file)/)",
  ],

But get a new, but similar error:

 FAIL   react  packages/remix-react/__tests__/transition-test.tsx
  ● Test suite failed to run

    TypeError: Class extends value undefined is not a constructor or null

      34 | class NodeRequest extends WebRequest {
      35 |   constructor(info: NodeRequestInfo, init?: NodeRequestInit) {
    > 36 |     super(info, init as RequestInit);
         |                                     ^
      37 |   }
      38 |
      39 |   public get headers(): WebHeaders {

This seems to relate to the types output by @remix-run/web-fetch:

export { default, fetch, Headers, Request, Response } from "./fetch.js";
export { ReadableStream, Blob, FormData } from "./package.js";
//# sourceMappingURL=lib.node.d.ts.map

node_modules/@remix-run/web-fetch/dist/src/lib.node.d.ts

fetch.js and package.js do not exist in the distributed @remix-run/web-fetch and likely should be pointing to fetch.d.ts and package.d.ts

edit: opened as #3402

@MichaelDeBoey
Copy link
Member

@penx Now that GoogleCloudPlatform/functions-framework-nodejs#461 is merged, do we still need jest v28?

@penx
Copy link
Contributor Author

penx commented Jun 6, 2022

yes, as per #3402 (comment)

Here's the test output on my firebase-adapter branch (used for #3397) with Jest@27 and "@google-cloud/functions-framework": "^3.1.2",

Screenshot 2022-06-06 at 22 23 14

@MichaelDeBoey
Copy link
Member

Hi @penx!

Are you still interested in getting this one merged?

If so, please rebase onto latest dev & resolve conflicts

Pinging @jenseng as well as I know he was looking into some Jest-related things as well

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 1, 2023
@penx
Copy link
Contributor Author

penx commented May 1, 2023

@MichaelDeBoey sure I’ll try find some time this Wednesday to clean up this and my unrelated examples PR

@penx
Copy link
Contributor Author

penx commented May 3, 2023

@MichaelDeBoey I've rebased, but am pretty sure this is still blocked by #3402. I had opened remix-run/web-std-io#11 which was approved almost a year ago but not merged.

@changeset-bot
Copy link

changeset-bot bot commented May 3, 2023

⚠️ No Changeset found

Latest commit: 397e179

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 3, 2023
@jenseng
Copy link
Contributor

jenseng commented May 5, 2023

@MichaelDeBoey thanks for the ping! I'm mostly interested in getting jest/jsdom upgraded on the react-router side, but good to see if happening here too ❤️

@brophdawg11 brophdawg11 added the dependencies Pull requests that update a dependency file label Jul 10, 2023
@MichaelDeBoey
Copy link
Member

Closing in favor of #7220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants