josdejong / svelte-jsoneditor

A web-based tool to view, edit, format, repair, query, transform, and validate JSON
https://jsoneditoronline.org
Other
816 stars 108 forks source link

Running Jest doesn't find 'vanilla-jsoneditor' module #334

Closed Rob-Negrete closed 7 months ago

Rob-Negrete commented 8 months ago

Hi @josdejong - I'm moving from the old jsoneditor to vanilla-jsoneditor. It was pretty easy, and the component works on runtime as a charm.

However, there's some weird issue when running test cases with jest.

My implementation is basically the same as JSONEditorReact.tsx. Then, when I run tests, this error is thrown:

 ● Test suite failed to run

    Cannot find module 'vanilla-jsoneditor' from 'JsonEditorReact.tsx'

    Require stack:
     JsonEditorReact.tsx

      at Resolver._throwModNotFoundError (../../../node_modules/jest-resolve/build/resolver.js:427:11)

I've installed vanilla-jsoneditor": "^0.18.11 and included as part of my dependencies.

"dependencies": {
    "react": "^16.14.0",
     ....
    "vanilla-jsoneditor": "^0.18.11"
  },

Accordingly, to what I've read, this can be related to jest-resolver not being able to resolve the types / mapping.

Am I missing something?

Thanks in advance.

josdejong commented 7 months ago

Thanks for reporting and working on a fix in #335.

How can we reproduce this issue exactly?

Do you have any pointers of other projects having similar issues with Jest? I would like to better understand what's going on.

Rob-Negrete commented 7 months ago

Hi @josdejong,

it’s a monorepo project having several modules. I’ve added several libs recently, however this is the only one lib having the issue.

It is like if the module wasn’t being detected or something at least at testing time.

josdejong commented 7 months ago

It is like if the module wasn’t being detected or something at least at testing time.

Indeed, and the question is why. How can I reproduce this issue?

Rob-Negrete commented 7 months ago

I tried to reproduce it on a separated project (standalone without monorepo) but worked fine. So, I'll to set up a tiny monorepo project and try to reproduce the error on it. If I had success, I'll share it to you.

josdejong commented 7 months ago

Thanks Rob!

Rob-Negrete commented 7 months ago

I'd sent you an invitation to my repo which you can use it to reproduce the error.

josdejong commented 7 months ago

Thanks for setting up the demo, I can reproduce the issue in your setup. It's quite a complex setup though 😅 .

I've done a bit of digging. The root cause seems to be that Jest does not have full ESM support (it is still marked experimental).

  1. Your solution to add a require field (CommonJS) in the package.json that points to the ESM file indeed works, see https://stackoverflow.com/a/75595580/1262753. It feels odd though to do that, I'm not sure whether other tools will get confused because of that, effectively breaking more than we fix with it.
  2. Various discussions suggest adding certain config for jest and ts-jest:
  3. To verify whether Jest's limited ESM support is indeed the cause, it may help to see whether other ESM only libraries give the same issue. If that is the case, we know that it isn't a vanilla-jsoneditor related issue. And if they do not give the same issue, we can have a look at their package.json to see what's the difference.

I haven't found a solution yet except for the require field hack. I have to say, I've intensively used a lot of different test libraries in the past, and most of them work great, only Karma and Jest where always giving me troubles. Jest has difficulties with TS and ESM and everything, therefore I moved away from it. I'm mostly using vitest nowadays, works like a charm.

I think there are three options to solve this issue:

  1. File a bug with Jest about this issue with an ESM only library.
  2. vanilla-jsoneditor is currently ESM only. Adding CommonJS support would solve the issue with Jest
  3. In your project, switch to an other test framework that works fine with ESM libraries.
Rob-Negrete commented 7 months ago

Hi @josdejong, thanks for following this up :)

Can you tell me more about option 2, please?

Adding CommonJS support would solve the issue with Jest

josdejong commented 7 months ago

If vanilla-jsoneditor would contain a CommonJS version of the library (alongside ESM), we can let this require in the package.json point to the actual CommonJS version of the code.

However, this does not have my preference. in general I would love the JS ecosystem to move away from CommonJS and adopt ESM, not the other way around. The current hybrid situation is a big pain. That's why so far I haven't provided a CommonJS version. See: #196

Rob-Negrete commented 7 months ago

Got it.

I'll try these configs you mentioned:

Various discussions suggest adding certain config for jest and ts-jest:
  + Use a --experimental-vm-modules flag, see https://jestjs.io/docs/ecmascript-modules.
  + Configure the useESM option of ts-node, see https://github.com/jestjs/jest/issues/13739#issuecomment-1655607116 and https://kulshekhar.github.io/ts-jest/docs/guides/esm-support/.
  + Configure a ts-jest/presets/default-esm preset, see
https://github.com/jestjs/jest/issues/13739#issuecomment-1655663846

And let it know if any of them works.

Otherwise, I might go back to jsoneditor 😞.

josdejong commented 7 months ago

As a workaround, you could also create a script that copies node_modules/vanilla-jsoneditor/* into some folder /src/assets/vanilla-jsonedtor, and then use the files from there: import { JSONEditor } from './src/assets/vanilla-jsoneditor/index.js'. It's ugly but it should work.

Rob-Negrete commented 7 months ago

Pretty odd, yeh. But, might help for a while.

Thanks.

Rob-Negrete commented 7 months ago

Sadly, none of them worked :(

Rob-Negrete commented 7 months ago

Compared against @tinyhttp/app which it only has this:

"exports": "./dist/index.js",

instead of something like this:

"exports": {
    ".": {
      "types": "./index.d.ts",
      "module": "./index.js"
    },
    "./CHANGELOG.md": "./CHANGELOG.md",
    "./index.d.ts": "./index.d.ts",
    "./index.js": "./index.js",
    "./index.js.map": "./index.js.map",
    "./LICENSE.md": "./LICENSE.md",
    "./README.md": "./README.md",
    "./SECURITY.md": "./SECURITY.md",
    "./themes/jse-theme-dark.css": "./themes/jse-theme-dark.css",
    "./themes/jse-theme-default.css": "./themes/jse-theme-default.css",
    "./package.json": "./package.json"
  },

By doing this, it seems to work.

What do you think about it?

josdejong commented 7 months ago

Ahh, that is a good idea! We have to make sure that TypeScript can still find the types (should be the case because of the types in the root of package.json, but we have to check that.

Rob-Negrete commented 7 months ago

Awesome! I tested it on my setup and it worked. Feel free to use it for your tests.

josdejong commented 7 months ago

😎 I'll do some testing next week, this looks promising.

josdejong commented 7 months ago

This issue should be fixed now in v0.19.0, can you give that a try @Rob-Negrete ?

Rob-Negrete commented 7 months ago

Sure, I'll try it today and let you know ;)

Rob-Negrete commented 7 months ago

Yei! It worked :)

josdejong commented 7 months ago

😎 Awesome!