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.

Sign up for GitHub

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

Jump to bottom

Use jest async transformers when available #25

Closed
benmccann opened this issue Dec 7, 2020 · 21 comments · Fixed by #57
Closed

Use jest async transformers when available #25

benmccann opened this issue Dec 7, 2020 · 21 comments · Fixed by #57

Comments

@benmccann
Copy link
Collaborator

This can't be done until jestjs/jest#9504 is completed (related PR pending jestjs/jest#9889). Filing here to track though

In order support preprocessing this library needs to spin up a new node instance for each source file which seems like it should have a negative performance impact.

One the jest work is done to support async transformers then we should use those here
@benmccann benmccann mentioned this issue Dec 7, 2020
Closed
@mihar-22
Copy link
Collaborator

mihar-22 commented Dec 8, 2020

Awesome thanks for setting this up @benmccann!

@benmccann
Copy link
Collaborator Author

Support now exists in Jest! However, it hasn't yet been released yet, so we'll need to wait a little in order to be able to use it

@rossyman rossyman mentioned this issue Mar 29, 2021
Merged
1 task
@sebastianrothe sebastianrothe mentioned this issue May 10, 2021
Closed
@sebastianrothe sebastianrothe mentioned this issue May 25, 2021
Merged
@sebastianrothe
Copy link
Collaborator

sebastianrothe commented May 25, 2021 •
edited

Jest 27 has just been released. Once my PR #46 has been merged, I will give it a try.

@benmccann
Copy link
Collaborator Author

I was just curious @sebastianrothe if this is something you're still planning to give a go of?

@sebastianrothe
Copy link
Collaborator

I was just curious @sebastianrothe if this is something you're still planning to give a go of?

I'd love to, but my project at work takes up all my time. I'll try to give it a go at the beginning of next month.

@benmccann
Copy link
Collaborator Author

benmccann commented Aug 1, 2021 •
edited

Looks like this will probably depend on #55. There's a PR for it though: #56

@benmccann benmccann mentioned this issue Aug 1, 2021
Merged
@mihar-22
Copy link
Collaborator

mihar-22 commented Aug 2, 2021 •
edited

Extending on a few things @benmccann noted here that I agree with.

If we want to support async transforms and not tie ourselves to Jest versioning, we'll need to support both sync and async transformers for the time being until a reasonable time where we can deprecate sync transformers. We can't force existing codebases using 26.x to upgrade, and if they decide not to upgrade they'll miss bug fixes or any new features. Thus, we need to support both for now.

I'm not sure what the best way will be to determine if async transforms are supported. Ideas include:

  • Node environment variable that indicates current Jest version?
  • Extracting Jest version from pacakge.json?
  • Some kind of check for async transformer support?

Keep in mind, it may executed inside a monorepo, and note any existing svelte-jester options that may change final transformation.

@benmccann
Copy link
Collaborator Author

We can't force existing codebases using 26.x to upgrade

I'm not sure it'd be much of an imposition to ask people to upgrade to v27.x. I just took a look at the breaking changes in v27 and they seem quite minor: https://jestjs.io/blog/2021/05/25/jest-27#features-coming-with-breaking-changes

@mihar-22
Copy link
Collaborator

mihar-22 commented Aug 2, 2021

I'll be honest I didn't actually look at the breaking changes. There seems to be nothing serious, I was thinking of a few other things outside of the noted breaking changes, but again nothing of importance.

If it all seems okay, we can just ignore what I said and next major version of svelte-jester can simply transition to async only transformers and bump minimum version for Jest in peer deps to >=27.

@sebastianrothe
Copy link
Collaborator

Extending on a few things @benmccann noted here that I agree with.

If we want to support async transforms and not tie ourselves to Jest versioning, we'll need to support both sync and async transformers for the time being until a reasonable time where we can deprecate sync transformers. We can't force existing codebases using 26.x to upgrade, and if they decide not to upgrade they'll miss bug fixes or any new features. Thus, we need to support both for now.

I'm not sure what the best way will be to determine if async transforms are supported. Ideas include:

* Node environment variable that indicates current Jest version?

* Extracting Jest version from `pacakge.json`?

* Some kind of check for async transformer support?

Keep in mind, it may executed inside a monorepo, and note any existing svelte-jester options that may change final transformation.

From my understanding, we can keep the existing code and just write a new async transformer. I think I already did some steps to extract functionality into seperate files. The only difference will be, how the transformer is called. Either as a subprocess with node or as an async function.

@benmccann
Copy link
Collaborator Author

I don't understand why we'd want to keep the existing code. Calling as a subprocess with node is purely worse. We should just always return an async transformer

@benmccann benmccann mentioned this issue Aug 2, 2021
Merged
@sebastianrothe
Copy link
Collaborator

Well, Jest <27 doesn't understand async transformers. If we rewrite the code, we loose compatibility with Jest 26.x
It is fine to me, but I just wanted to point that out.

Guess, people who need to run Jest 26.x can always use an older version, right.

@benmccann
Copy link
Collaborator Author

Yeah, that's fine. We listed jest >= 27 as a peer dependency:

https://github.com/mihar-22/svelte-jester/blob/1052bed4eb2c3c7c1cc6739ee21363de38f4f7c4/package.json#L46

@gorjan-mishevski
Copy link

gorjan-mishevski commented Aug 9, 2021 •
edited

It seems this is not quite resolved.
I am getting the following error:
Jest: synchronous transformer node_modules/svelte-jester/src/transformer.js must export a "process" function.

"svelte-jester": "2.0.1"
"jest": "27.0.6"
"svelte": "3.41.0"

jest config

 transform: {
    "^.+\\.ts$": "ts-jest",
    "^.+\\.svelte$": [
      "svelte-jester",
      {
        "preprocess": true
      }
    ],
  },

@rkennel
Copy link

rkennel commented Aug 21, 2021

Yes, I am seeing same error as @gorjan-mishevski. Same jest config file

svelte-jester@2.0.1
jest@27.0.6
svelte@3.42.2

@ritchieanesco
Copy link

+1. Also same config.

svelte-jester@2.0.1
jest@27.1.1
svelte@3.42.5

@benmccann
Copy link
Collaborator Author

Please use the latest version: 2.1.2

@ritchieanesco
Copy link

@benmccann updating to 2.1.2 now gives me
TypeError: Jest: a transform must export a `process` or `processAsync` function. which was raised in this issue #74. Has this issue regressed?

@rmunn
Copy link

rmunn commented Sep 16, 2021

@ritchieanesco #74 was fixed by #75 but that fix hasn't made its way into a released version yet. I don't know why, because 2.1.2 is unusable, so I'd have thought that 2.1.3 would be more urgent. But once 2.1.3 comes out, it will work. I've verified the fix in #75 by cloning the svelte-jester repo and putting "svelte-jester": "../svelte-jester" in my package.json, and then I could successfully use jest.unstable_mockModule in my Svelte unit tests.

So wait for 2.1.3 to come out and it should work for you.

@sebastian-meier sebastian-meier mentioned this issue Sep 16, 2021
Open
@adam-kov
Copy link

I had the same problem as @gorjan-mishevski but 2.1.3 is out now for svelte-jester. I updated jest as well to the currently latest version (27.2.0) and now I get a new error:

Cannot find module './preprocess.js'
Require stack:
      node_modules/svelte-jester/dist/transformer.cjs
      node_modules/@jest/transform/node_modules/jest-util/build/requireOrImportModule.js
      node_modules/@jest/transform/node_modules/jest-util/build/index.js
      node_modules/@jest/transform/build/ScriptTransformer.js
      node_modules/@jest/transform/build/index.js
      node_modules/jest-runtime/build/index.js
      node_modules/@jest/core/build/cli/index.js
      node_modules/@jest/core/build/jest.js
      node_modules/jest/node_modules/jest-cli/build/cli/index.js
      node_modules/jest/node_modules/jest-cli/bin/jest.js
      node_modules/jest/bin/jest.js

      at Object.processSync [as process] (node_modules/svelte-jester/dist/transformer.cjs:115:32)
      at ScriptTransformer.transformSource (node_modules/@jest/transform/build/ScriptTransformer.js:612:31)
      at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:758:40)
      at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:815:19)

@colehpage
Copy link

I had the same problem as @gorjan-mishevski but 2.1.3 is out now for svelte-jester. I updated jest as well to the currently latest version (27.2.0) and now I get a new error:

Cannot find module './preprocess.js'
Require stack:
      node_modules/svelte-jester/dist/transformer.cjs
      node_modules/@jest/transform/node_modules/jest-util/build/requireOrImportModule.js
      node_modules/@jest/transform/node_modules/jest-util/build/index.js
      node_modules/@jest/transform/build/ScriptTransformer.js
      node_modules/@jest/transform/build/index.js
      node_modules/jest-runtime/build/index.js
      node_modules/@jest/core/build/cli/index.js
      node_modules/@jest/core/build/jest.js
      node_modules/jest/node_modules/jest-cli/build/cli/index.js
      node_modules/jest/node_modules/jest-cli/bin/jest.js
      node_modules/jest/bin/jest.js

      at Object.processSync [as process] (node_modules/svelte-jester/dist/transformer.cjs:115:32)
      at ScriptTransformer.transformSource (node_modules/@jest/transform/build/ScriptTransformer.js:612:31)
      at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:758:40)
      at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:815:19)

@n00pper it was left out of the published version but is being addressed 👍
#81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Avoid creating new node processes by leveraging processAsync benmccann/svelte-jester
9 participants
@rmunn @benmccann @sebastianrothe @ritchieanesco @colehpage @gorjan-mishevski @mihar-22 @rkennel @adam-kov