DomParfitt / graphviz-react

React component for displaying Graphviz graphs
MIT License
101 stars 21 forks source link

Unable to import package #47

Open cyberixae opened 2 years ago

cyberixae commented 2 years ago

The current release package seems to be broken. I did manage to use the package with TypeScript since it somehow miraculously transforms the code. However, I couldn't get my tests to work with ts-jest which made me research the issue.

In its current form Node.js crashes when you try to import the module.

$ node
Welcome to Node.js v16.13.0.
Type ".help" for more information.
> await import('graphviz-react')
(node:13519) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
/XXXXX/node_modules/graphviz-react/lib/Graphviz.js:1
import * as React from 'react';
^^^^^^

The Node.js crash can be fixed by adding "type": "module", to package.json. That will make basic importing work.

$ node
Welcome to Node.js v16.13.0.
Type ".help" for more information.
> await import('graphviz-react')
[Module: null prototype] {
  Graphviz: [class Graphviz extends Component] {
    count: 0,
    defaultOptions: { fit: true, height: 500, width: 500, zoom: false }
  },
  default: [class Graphviz extends Component] {
    count: 0,
    defaultOptions: { fit: true, height: 500, width: 500, zoom: false }
  }
}

However, fixing the package.json file did not fix my tests, so I continued my research. Even after the fix, importing the package in ts-node still fails for some reason.

$ ts-node
> await import('graphviz-react')
Uncaught:
Error [ERR_REQUIRE_ESM]: require() of ES Module /XXXXX/node_modules/graphviz-react/lib/Graphviz.js not supported.
Instead change the require of Graphviz.js in null to a dynamic import() which is available in all CommonJS modules.
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at /XXXXX/<repl>.ts:4:14
    at require (node:internal/modules/cjs/helpers:102:18)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1128:19)
    at new NodeError (node:internal/errors:371:5)
    at __node_internal_captureLargerStackTrace (node:internal/errors:464:5) {
  code: 'ERR_REQUIRE_ESM'
}
cyberixae commented 2 years ago

Pull request #48 adds the missing type definition solving at least one part of the puzzle.

DomParfitt commented 2 years ago

Hi @cyberixae , thanks for raising an issue.

Would you be able to provide some more detail on what it is you're trying to do and what the issue you're experiencing is? It would be good to have all the information before making any decisions on potential bug fixes.

cyberixae commented 2 years ago

Maybe I could create a demo repository. It is a bit complicated with several things interacting.

cyberixae commented 2 years ago

Here is a minimal example that reproduces the problem https://github.com/cyberixae/graphviz-import-study

DomParfitt commented 2 years ago

Cheers @cyberixae, I'll take a look when I get a moment.

lyleaigbedion commented 1 year ago

Has any progress been made on this? I'm running into the same issue.

lyleaigbedion commented 1 year ago

Actually, you can add "transformIgnorePatterns": [ "/node_modules/(?!graphviz-react)" ] to your jest configuration to prevent this issue. I ran into something like this while implementing React Drag and Drop https://github.com/react-dnd/react-dnd/issues/3443

cyberixae commented 1 year ago

I added this line to the jest config of the graphviz-import-study from above but that did not seem to fix the problem.

DomParfitt commented 1 year ago

Sorry this slipped off my radar. @cyberixae I've merged your PR and published a new version. Can you try updating and see if that fixes your issue?

cyberixae commented 1 year ago

I updated the graphviz-import-study to use the new package. This did not fix the issue I have running the tests. I still get the same error. Adding transformIgnorePatterns to jest config doesn't seem to help either.

      at testStorySnapshots (node_modules/@storybook/addon-storyshots/dist/ts3.9/api/index.js:86:15)
      at Object.<anonymous> (src/__tests__/stories.ts:22:15)

  console.warn
    Unexpected error while loading ./components/__stories__/graphviz.tsx: SyntaxError: Cannot use import statement outside a module

      at Object.warn (node_modules/@storybook/client-logger/dist/cjs/index.js:67:65)
      at node_modules/@storybook/core-client/dist/cjs/preview/loadCsf.js:92:34
          at Array.forEach (<anonymous>)
      at node_modules/@storybook/core-client/dist/cjs/preview/loadCsf.js:85:20
          at Array.forEach (<anonymous>)
      at node_modules/@storybook/core-client/dist/cjs/preview/loadCsf.js:84:12
      at ConfigApi.configure (node_modules/@storybook/client-api/dist/cjs/config_api.js:26:7)

However, the problem related to node command line is now fixed as expected.

 $ node
Welcome to Node.js v16.13.0.
Type ".help" for more information.
> await import('graphviz-react')
[Module: null prototype] {
  Graphviz: [class Graphviz extends Component] {
    count: 0,
    defaultOptions: { fit: true, height: 500, width: 500, zoom: false }
  },
  default: [class Graphviz extends Component] {
    count: 0,
    defaultOptions: { fit: true, height: 500, width: 500, zoom: false }
  }
}
>
lyleaigbedion commented 1 year ago

I got past the error by adding transform: { '^.+\\.(ts|tsx|js)$': 'ts-jest'} and transformIgnorePatterns: [ '/node_modules/(?!graphviz-react)' ] to the jest.config.js file and in your tsconfig.json "allowJs": true. The test fails btw, I dont know storybook so I don’t know if that's supposed to happen.

cyberixae commented 1 year ago

@lyleaigbedion Running Jest with --updateSnapshot (or -u) is supposed to create test renderings of the user interface components. Running Jest without the -u flag is supposed to check that the rendered components still match the created test renderings. This is done to detect if something unintentionally changes the output. The problem I am experiencing is that even updating the snapshots fails. Updating the snapshots should never fail since it overwrites all previously created snapshots.