Add back process method for CJS support #71

benmccann · 2021-09-04T23:37:56Z

It turns out that processAsync is not supported in CJS mode, so we still need the old process method

sebastianrothe · 2021-09-05T08:42:18Z

Can you find some reference in the docs from Jest that this is really the case?

I am not sure about this, because I think I remember having seen asynchronous transformer and CJS (vue-jest or something).

sebastianrothe · 2021-09-05T09:26:04Z

I think, ESM support in Jest is experiment (according to docs and because of all the flags involved), which means it is still written in CJS. Now async transformers were introduced in v27. To me this means Async support in CJS should be superior.

I think we have to look for the root of the problem, once we are sure the transpilation worked.

mszkb · 2021-09-05T12:00:31Z

@benmccann pls add import { execSync } from 'child_process' in transformer.js file.

Another thing: For some reason I need to add the jsdom environment in package.json to run my tests, like:
"scripts": { "test": "jest --env=jsdom" }

In svelte-jester 1.8.2 this was not necessary, do you know why is that?

benmccann · 2021-09-05T13:27:22Z

Can you find some reference in the docs from Jest that this is really the case?

Unfortunately the docs for processAsync are quite lacking (there were none until I added some and I didn't know about that restriction, so I didn't mention it), but here's a reference for it: jestjs/jest#11458 (comment)

Another thing: For some reason I need to add the jsdom environment in package.json to run my tests, like:
"scripts": { "test": "jest --env=jsdom" }
In svelte-jester 1.8.2 this was not necessary, do you know why is that?

That's not related to svelte-jester. It's because the default environment changed to Node in Jest 27

benmccann · 2021-09-05T13:30:35Z

pls add import { execSync } from 'child_process' in transformer.js file.

done. thanks for catching that! I'd tested this, but I guess not with the preprocess option, so I missed that

sebastianrothe · 2021-09-05T18:27:09Z

Unfortunately the docs for processAsync are quite lacking (there were none until I added some and I didn't know about that restriction, so I didn't mention it), but here's a reference for it: facebook/jest#11458 (comment)

Hm, this refers only to the require being synchronously. But this is just the module resolution, not the execution of the function. If this is true, then no one using a CJS library would be able to use an async function. Which I don't think is true.

I will have a look at the code from jest calling the transformer now.

sebastianrothe · 2021-09-05T18:33:29Z

This is the code loading the transformer: https://github.com/facebook/jest/blob/master/packages/jest-transform/src/ScriptTransformer.ts#L268
It is a check, if the export is createTransformer or process* is a function and later this function is called with await (https://github.com/facebook/jest/blob/3c511fd1a0a2e2d4c7a3c6880ac48e23cc807fa9/packages/jest-transform/src/ScriptTransformer.ts#L676) or not (https://github.com/facebook/jest/blob/3c511fd1a0a2e2d4c7a3c6880ac48e23cc807fa9/packages/jest-transform/src/ScriptTransformer.ts#L705).

Another example is this e2e-test: https://github.com/facebook/jest/blob/master/e2e/transform/async-transformer/my-transform.cjs (Okay, the test itself is ESM)

sebastianrothe · 2021-09-05T18:52:41Z

Hm, still unsure. I am preparing a test case to validate this.

Jest references async transformers a lot with ESM:

mszkb · 2021-09-05T20:53:04Z

pls add import { execSync } from 'child_process' in transformer.js file.

done. thanks for catching that! I'd tested this, but I guess not with the preprocess option, so I missed that

Thanks, tested and new changes on the branch are working

benmccann · 2021-09-06T03:51:58Z

@sebastianrothe I don't know the Jest internals well enough to have any idea of whether there might be a way to get processAsync to run in CJS mode.

The way the code is being called without this PR is:

    at ScriptTransformer.transformSource (node_modules/@jest/transform/build/ScriptTransformer.js:584:7)
    at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:759:40)
    at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:816:19)
    at Runtime.transformFile (node_modules/jest-runtime/build/index.js:1463:47)
    at Runtime._execModule (node_modules/jest-runtime/build/index.js:1380:34)
    at Runtime._loadModule (node_modules/jest-runtime/build/index.js:1034:12)
    at Runtime.requireModule (node_modules/jest-runtime/build/index.js:866:12)
    at jestAdapter (node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:13)
    at runTestInternal (node_modules/jest-runner/build/runTest.js:389:16)
    at runTest (node_modules/jest-runner/build/runTest.js:481:34)

(line number might be off by a line or two as I'm running a locally-modified version)

There are async versions of many of these methods. I.e. transformSourceAsync, _transformAndBuildScriptAsync, transformAsync, transformFileAsync. But for some reason those aren't being called.

Maybe you can file an issue to ask in the Jest issue tracker or something. I'm afraid I don't have any insight other than seeing the comment where he said "async transform is only supported when using ESM"

sebastianrothe · 2021-09-06T08:07:56Z

Maybe you can file an issue to ask in the Jest issue tracker or something. I'm afraid I don't have any insight other than seeing the comment where he said "async transform is only supported when using ESM"

I did ask him in the Jest issue (jestjs/jest#11458 (comment))

Nonetheless, I do have a test which consumes the CJS build and it is passing. Let my try to create a new Svelte project and try it there as well.

mihar-22 · 2021-09-08T01:47:54Z

At this point both you @benmccann and @sebastianrothe have a better understanding of what's happening. I'm completely hands off so feel free to merge and I'll release as PR's go through. If you need any permissions let me know. We could even automate release with GitHub CI. Happy to set it up if needed.

sebastianrothe · 2021-09-08T18:47:34Z

jestjs/jest#11458 (comment) means, we need to bring the old code back for the CJS version.

I would like to bring back the tests for the syncTransformer as well. I will try to add those commits tomorrow.

sebastianrothe · 2021-09-11T12:55:41Z

I'm having some problems with the typescript test. We should really use the Github Actions running our tests on every PR.

benmccann · 2021-09-11T13:35:41Z

We could possibly merge this PR as is (all tests are passing with it) to get people unbroken and then handle adding the tests in a new PR. I agree it'd be nice to run the tests on the CI

sebastianrothe · 2021-09-11T15:11:01Z

I'm having some problems with the typescript test. We should really use the Github Actions running our tests on every PR.

Found it.

sebastianrothe · 2021-09-11T15:12:42Z

We could possibly merge this PR as is (all tests are passing with it) to get people unbroken and then handle adding the tests in a new PR. I agree it'd be nice to run the tests on the CI

Well, the tests run only for the async build. So I think it is important to have them pass for the sync build as well.

I will have some more time for this in the evening (in 3 hours).

… be a svelte-compile error

remove duplicate test

add esInterop for cjs format export createTransformer for process function

sebastianrothe · 2021-09-11T20:56:35Z

We could possibly merge this PR as is (all tests are passing with it) to get people unbroken and then handle adding the tests in a new PR. I agree it'd be nice to run the tests on the CI

I added my commits to yours. Can you please have a look: https://github.com/mihar-22/svelte-jester/pull/71/files/4c72f25507b1fc5c457ef2da80bd62524990cfa5..75c83c690de2156f37fc577912c81fc05d2937df

removed esbuild, because with Jest supporting ESM it is not needed anymore for the tests
svelte-preprocess > 4.7.4 gave me errors on the test with Typescript -> needs more investigation, because they changed the typescript support
for cjs, we can't export const process because this variable is already taken by node and we need it to pass env-vars to the child process

benmccann · 2021-09-11T21:44:52Z

package.json

@@ -6,8 +6,7 @@
  "module": "dist/transformer.mjs",
  "exports": {
    "import": "./dist/transformer.mjs",
-    "require": "./dist/transformer.cjs",
-    "./package.json": "./package.json"


some plugins like rollup-plugin-svelte will log a warning if package.json is not exported, so I added it back. I updated the default export, so if you were having any issue hopefully that will solve it

Okay, vscode gave me an error on this line.

benmccann · 2021-09-11T21:53:50Z

package.json

    "release": "npm run validate && standard-version"
  },
  "peerDependencies": {
    "jest": ">= 27",
-    "svelte": ">= 3"
+    "svelte": ">= 3",
+    "svelte-preprocess": "<=4.7.4"


I think this will force a dependency on svelte-preprocess even for projects written purely in JS that don't need to do any preprocessing

4.8.0 works for me. I think the issue was just introduced last week: https://github.com/sveltejs/svelte-preprocess/blob/5aa60cb86c89c0649a548507f6d142937f76df7a/src/transformers/typescript.ts#L26

I sent a PR to svelte-core that allowed me to track down this issue: sveltejs/svelte#6722

I think this will force a dependency on svelte-preprocess even for projects written purely in JS that don't need to do any preprocessing

Ups, you are right.

Fix for the svelte-preprocess issue is here: sveltejs/svelte-preprocess#407. We can unpin svelte-preprocess in this project after that's committed

benmccann mentioned this pull request Sep 4, 2021

svelte-jester 2.x does not work rossyman/svelte-add-jest#19

Closed

sebastianrothe self-requested a review September 5, 2021 13:01

Add back process method for CJS support

de4a30a

benmccann force-pushed the cjs branch from 0eb10a5 to de4a30a Compare September 5, 2021 13:29

update docs

4c72f25

benmccann mentioned this pull request Sep 9, 2021

[bug: #19, #22] Fix examples and v27 Jest support / ESM rossyman/svelte-add-jest#23

Merged

sebastianrothe added 4 commits September 10, 2021 22:36

remove not allowed export

1eb6fa7

upgrade packages

1112e81

rename rollup config

f640b8a

remove run from some npm commands

391f57e

sebastianrothe added 3 commits September 11, 2021 22:49

test

020cf66

lock svelte-preprocess version to 4.7.4, because otherwise there will…

1ac5bc1

… be a svelte-compile error

no need to transform anything, because jest supports ESM now

4d97253

sebastianrothe added 9 commits September 11, 2021 22:49

no need to transform anything

95aa533

typescript is implicit

a2e94c9

run tests before releasing

beab491

run build before testing

0eff1e1

revert deleting tests for cjs sync transformer

a2ed8c0

fix import

024a9a6

remove duplicate test

fix reserved variable process

0447ce9

add esInterop for cjs format export createTransformer for process function

use named exports only

5225a86

current package-lock

75c83c6

sebastianrothe approved these changes Sep 11, 2021

View reviewed changes

add back package.json to exports map

256bb12

benmccann commented Sep 11, 2021

View reviewed changes

remove svelte-preprocess peer dep and bump svelte-preprocess

3a52cb9

benmccann merged commit fe188c4 into svelteness:master Sep 11, 2021

mihar-22 pushed a commit that referenced this pull request Sep 12, 2021

fix: add back process method for CJS support (#71)

6ee7c5c

colehpage mentioned this pull request Sep 19, 2021

Cannot find module './preprocess.js' #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add back process method for CJS support #71

Add back process method for CJS support #71

benmccann commented Sep 4, 2021

sebastianrothe commented Sep 5, 2021 •

edited

Loading

sebastianrothe commented Sep 5, 2021 •

edited

Loading

mszkb commented Sep 5, 2021 •

edited

Loading

benmccann commented Sep 5, 2021

benmccann commented Sep 5, 2021

sebastianrothe commented Sep 5, 2021 •

edited by benmccann

Loading

sebastianrothe commented Sep 5, 2021 •

edited

Loading

sebastianrothe commented Sep 5, 2021

mszkb commented Sep 5, 2021

benmccann commented Sep 6, 2021

sebastianrothe commented Sep 6, 2021 •

edited by benmccann

Loading

mihar-22 commented Sep 8, 2021

sebastianrothe commented Sep 8, 2021

sebastianrothe commented Sep 11, 2021

benmccann commented Sep 11, 2021

sebastianrothe commented Sep 11, 2021

sebastianrothe commented Sep 11, 2021

sebastianrothe commented Sep 11, 2021

benmccann Sep 11, 2021

sebastianrothe Sep 12, 2021

benmccann Sep 11, 2021

benmccann Sep 11, 2021

benmccann Sep 11, 2021

sebastianrothe Sep 12, 2021

benmccann Sep 12, 2021

Add back process method for CJS support #71

Add back process method for CJS support #71

Conversation

benmccann commented Sep 4, 2021

sebastianrothe commented Sep 5, 2021 • edited Loading

sebastianrothe commented Sep 5, 2021 • edited Loading

mszkb commented Sep 5, 2021 • edited Loading

benmccann commented Sep 5, 2021

benmccann commented Sep 5, 2021

sebastianrothe commented Sep 5, 2021 • edited by benmccann Loading

sebastianrothe commented Sep 5, 2021 • edited Loading

sebastianrothe commented Sep 5, 2021

mszkb commented Sep 5, 2021

benmccann commented Sep 6, 2021

sebastianrothe commented Sep 6, 2021 • edited by benmccann Loading

mihar-22 commented Sep 8, 2021

sebastianrothe commented Sep 8, 2021

sebastianrothe commented Sep 11, 2021

benmccann commented Sep 11, 2021

sebastianrothe commented Sep 11, 2021

sebastianrothe commented Sep 11, 2021

sebastianrothe commented Sep 11, 2021

benmccann Sep 11, 2021

Choose a reason for hiding this comment

sebastianrothe Sep 12, 2021

Choose a reason for hiding this comment

benmccann Sep 11, 2021

Choose a reason for hiding this comment

benmccann Sep 11, 2021

Choose a reason for hiding this comment

benmccann Sep 11, 2021

Choose a reason for hiding this comment

sebastianrothe Sep 12, 2021

Choose a reason for hiding this comment

benmccann Sep 12, 2021

Choose a reason for hiding this comment

sebastianrothe commented Sep 5, 2021 •

edited

Loading

sebastianrothe commented Sep 5, 2021 •

edited

Loading

mszkb commented Sep 5, 2021 •

edited

Loading

sebastianrothe commented Sep 5, 2021 •

edited by benmccann

Loading

sebastianrothe commented Sep 5, 2021 •

edited

Loading

sebastianrothe commented Sep 6, 2021 •

edited by benmccann

Loading