bkrem / react-d3-tree

:deciduous_tree: React component to create interactive D3 tree graphs
https://bkrem.github.io/react-d3-tree
MIT License
1.08k stars 269 forks source link

tree/index.js require() breaks with node 18, nextjs13, react18 - error recommends using import instead #418

Closed pgeday closed 1 year ago

pgeday commented 1 year ago

Are you reporting a bug, or opening a feature request?

Bug

What version of react-d3-tree are you using?

3.3.6

If react-d3-tree crashed with a traceback, please paste the full traceback below.

Server Error Error: require() of ES Module C:......\node_modules\d3-selection\src\index.js from C:....\node_modules\react-d3-tree\lib\Tree\index.js not supported. Instead change the require of C:....\node_modules\d3-selection\src\index.js in C:....\node_modules\react-d3-tree\lib\Tree\index.js to a dynamic import() which is available in all CommonJS modules.


Thanks bkrem for developing this package! It has been very useful.

I have recently updated my app to node 18, nextjs 13, react 18.
It was great to see that you recently solved the reDoS critical vulnerability in react-d3-tree. However, now it breaks my app (for some reason after a login). The error message references node_modules/react-d3-tree/lib/tree/index.js

The issue seems to be that it includes require() (lines 43-53). The latest above tools wants you to use import instead, as require is not supported by all tools. See error langauge below.

I wanted to bring your attention to this breaking issue, as it will likely cause problem to an increasing number of users when / if they upgrade their apps to the latest tech.

Thanks, Peter

smooth-lee commented 1 year ago

Hello, I had the same problem and solved it with the code I wrote below.

import dynamic from 'next/dynamic'; const Tree = dynamic(() => import('react-d3-tree'), { ssr: false });

I hope it helps you.

pmacom commented 1 year ago

Hmm, I don't think the above solution works. I'm getting the following error: `Error: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.

pgeday commented 1 year ago

Thanks for the idea. If I understand it correctly you would put this in a component on the UI side. However, my understanding of the issue is that react-d3-tree requires the d3 functions and capabilities in its index, instead of importing them. It seems as if the latest next.js and/or react interpreters do not like the 'required()' anymore, but prefer the newer import. When I tried to use import in the index.js referenced in the error, it indicated that I cannot use import outside of a component. So it goes beyond my current capabilities, hence I reached out to the creator of the package.

Did you mean with the dynamic import to only import the chart when needed, which is typically used to limit the package size and improve performance? Do you think that the error is generated when the server tries to create the pages, but there should not be any error if only the UI side creates it (hence the ssr false)?

Thanks, again for the ideas. I hope bkrem will also chime in at some point, as they know their package best. This will become a frequent bug as more folks transition to the latest tech. Cheers

pgeday commented 1 year ago

Hello, I had the same problem and solved it with the code I wrote below.

import dynamic from 'next/dynamic'; const Tree = dynamic(() => import('react-d3-tree'), { ssr: false });

I hope it helps you.

I tried this on the UI component side. While I have already been importing Tree (not require), I tried the dynamic import. Same issue comes up after logging into a new session with a user. The error log references using dynamic import within react-d3-tree index.js, but that file is not sent to the client side in my understanding.

I think the problem is around these lines within \node_modules\react-d3-tree\lib\Tree\index.js

var react_1 = importDefault(require("react")); var d3_hierarchy_1 = require("d3-hierarchy"); var d3_selection_1 = require("d3-selection"); var d3_zoom_1 = require("d3-zoom"); var lite_1 = require("dequal/lite"); var clone_1 = importDefault(require("clone")); var uuid_1 = require("uuid"); var TransitionGroupWrapper_1 = importDefault(require("./TransitionGroupWrapper")); var Node_1 = __importDefault(require("../Node")); var Link_1 = importDefault(require("../Link")); var globalCss_1 = __importDefault(require("../globalCss"));

bkrem commented 1 year ago

Welp, I already had a suspicion that going up two major versions d3-...@1 -> d3-...@3 went surprisingly smoothly to fix that aforementioned vulnerability.

Thanks @pgeday for making me aware of the breakage here with Next 13 and @smooth-lee for providing a possible workaround. I haven't had time to deep dive this yet due to work obligations.

Suggested workaround

I've been able to repro the issue in a fresh Next 13 repo, but @smooth-lee's suggestion of using dynamic imports seems to resolve it/the library loads as expected. Also more guidance on this approach here: https://www.freecodecamp.org/news/how-to-bypass-es-modules-error-in-next-js/

Solution

The root cause here is that *these `d3-packages only ship with ESM modules now fromv3onwards**, causing this conflict where react-d3-tree's CommonJS modules are trying to import an ESM module withrequire`.

I think the proper solution is to add ESM modules to react-d3-tree's package output, which should hopefully resolve this conflict and is probably overdue anyways.

Will try to add ESM module support this evening or this weekend. In the meantime please use the suggested workaround or share how you were able to resolve 🙏

Additionally if anyone is aware of how to fix this quickly at the library level, please feel free to open a PR and I'll make sure you get the appropriate credit!

pgeday commented 1 year ago

Hi bkrem, Thanks for your response. I changed the component that leveraged Tree, to start like this:

import React, { useState } from 'react'; import dynamic from 'next/dynamic' const Tree = dynamic(() => import('react-d3-tree'), { ssr: false });

The behavior remained the same. It works fine with or without the dynamic import IF I already have a session running. I see all the hierarchy charts without any problem. The error comes up when I log in with next-auth/credentials and I get a new session. Upon further testing, I noted the following: If I do not have a user and go to login page, log a new user in, there is no problem, the server does not break. However, the problem comes up if I am not logged in and I try to go to a page that is protected by the middleware.js within next.js to prompt the client to login first. After login with Next-Auth /credentials, as Next.js is trying to direct me to the page I wanted to open, then it breaks. This makes me wonder if the problem is only with react-d3-tree, or how Next-Auth works, or how I implemented Next-Auth and the middleware. Before the upgrades this did not happen, but I upgraded everything and this is the only break (that I noticed). I am not sure if it makes sense to ask the Next-Auth team as I guess I cannot give them a comment with an issue that references a third party package as the source of the error. I hope you have some closer working relationships with them as a package owner.

By the way, this package is great. What I like the most is that I can move the hierarchy and zoom in/out on a canvas. If I can recommend one thing, then it would be to work on how to show a hierarchy that is not deep but very broad (e.g., 2 levels 10-10 each), which makes it very wide in a vertical view, while it could be just deep. Thanks!

bkrem commented 1 year ago

This should now be fixed as of https://github.com/bkrem/react-d3-tree/releases/tag/v3.4.2

I've tested against Next 12 and 13. In both cases it should now simply resolve the ESM modules which do not have this awkward issue.

Please let me know if you're still encountering this issue and I'll reopen 👍


I originally shipped ESM outputs as 3.4.0 but my initial testing didn't reveal a whole different ESM-related issue (ERR_UNSUPPORTED_DIR_IMPORT), so I had to revert via 3.4.1.

Sorry about the version noise! Usually doesn't happen like this thankfully. The current solution to comply with ESM's exact path resolution is inelegant but it'll do for now.