mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.87k stars 35.39k forks source link

Nodes: Nodes system throws exceptions when used with vite, `functionNode.call is not a function` #27360

Open hybridherbst opened 11 months ago

hybridherbst commented 11 months ago

Description

I'm trying to use the Nodes system, including the MaterialX nodes, from a vite project.

Local development (unbundled) works fine, but bundling leads to errors. I was able to isolate and reproduce the issue, but I'm so far not sure if this is a three.js issue (some incorrect setup on the nodes) vs. a vite issue (something going wrong with bundling), or a combination thereof.

Reproduction steps

  1. Go to https://stackblitz.com/edit/vitejs-vite-9b37bu
    1. This roughly uses the code from https://threejs.org/examples/?q=materialx#webgl_nodes_materialx_noise
    2. There's no attempt to output something visually, it's just about getting things to run at all.
  2. Open the browser console
  3. Run npm run start
  4. Note no errors (code runs fine)
  5. Cancel the run by pressing Ctrl + C in the terminal
  6. Run npm run preview which builds the code and hosts the resulting bundled code
  7. In the console, note errors:
image

and when names are kept during bundling, and code is not minified:

image

I was able to track this down to FunctionNodes:

image

and functionNode.call being undefined in a production build vs. in local development, but I'm not sure what may be causing this.

I'm not aware of a workaround for how to use the nodes/MaterialX system with a bundler at the moment.

Live example

Screenshots

No response

Version

r159

Device

Desktop, Mobile

Browser

Chrome, Safari

OS

Windows, MacOS

hybridherbst commented 11 months ago

I think this may be caused by the Nodes system heavily using module side-effects, e.g. FunctionNode registers itself as a side-effect of importing the module: (I don't think that particular line is tree-shaken out though)

If I'm not mistaken that means treeshaking will, if no-one explicitly uses any of the other methods of FunctionNode, remove the entire module.

export default defineConfig(async ({ command }) => {
  return {
    esbuild: {
      minifyIdentifiers: false,
      keepNames: true,
    },
    build: {
      minify: false,
      rollupOptions: {
        treeshake: true, // change this to true or false and run `npm run preview` again and refresh the page
      },
    },
  };
});
marcofugaro commented 11 months ago

Hasn't this been solved in #27189?

hybridherbst commented 11 months ago

I tested on r159 and latest dev and the behaviour happens in both of them (the Stackblitz version above is on r159 release as well).

hybridherbst commented 11 months ago

Digging a bit deeper the problem got more weird: seems that while functionNode.call is proxied when running locally, the proxy getter isn’t called anymore in a build.

CodyJasonBennett commented 11 months ago

From https://github.com/mrdoob/three.js/pull/27189#issuecomment-1854016611:

Out of curiosity, was it even intentional that #26881 now requires these additional steps? I can't see a mention of it in the original PR so can only assume that was by accident. I think it would be better if there's no additional configuration needed.

From https://github.com/vitejs/vite/issues/15319#issuecomment-1855666500:

Turns out the three.js package already declares these particular files as having side effects in its package.json. "sideEffects": ["./examples/jsm/nodes/Nodes.js"],

Only that exact single module is side effectful, all the modules re-exported from there aren't; If some of them are indeed side effectful, you should mark them in sideEffects, in your case:

- "sideEffects": ["./examples/jsm/nodes/Nodes.js"],
+ "sideEffects": ["./examples/jsm/nodes/Nodes.js", "./examples/jsm/nodes/core/FunctionCallNode.js"],

I'll need to test globbing ./examples/jsm/nodes/* with Vite specifically, but best if we can avoid this altogether as per the first quote.

XiSenao commented 11 months ago

https://github.com/vitejs/vite/issues/15319#issuecomment-1855784471

hybridherbst commented 11 months ago

Thank you all!

@sunag @CodyJasonBennett I'd still be curious if it was even intentional that these modules now have side-effects in this way. I think it would be less confusing and less prone to breakage with various bundlers if that wouldn't be the case.

hybridherbst commented 11 months ago

I tested with the changes introduced in #27376 and the issue still happens.

@Mugen87 can you reopen this issue please?

sunag commented 11 months ago

I think that many of these problems were due to https://github.com/mrdoob/three.js/pull/25074, I was hoping to find an alternative solution, but it seems that the best option would be that the method chaining only works for OperatorNode, MathNode, ShaderNode that would be the most used.

XiSenao commented 11 months ago

I tested with the changes introduced in #27376 and the issue still happens.

@Mugen87 can you reopen this issue please?

By testing the repository you provided at https://stackblitz.com/edit/vitejs-vite-9b37bu (with addition of #27376 modification), it seems to be working fine. Can you provide a new minimal reproducible repository?

hybridherbst commented 11 months ago

@XiSenao Out of curiosity, how did you test that? (would you mind sharing your adjusted version?) I think for a proper test the modification needs to be in package.json of three, not in package.json of the parent project.

@sunag as MaterialX can be loaded at runtime, I think the nodes can't really be treeshaken anyways, right? Couldn't Nodes.js import all of them and thus ensure an order and avoid side-effects? (please ignore if that doesn't make sense, I'm just dipping my toes into the whole side effects thing)

XiSenao commented 11 months ago

A relatively simple way to access the project you provided at https://stackblitz.com/edit/vitejs-vite-9b37bu, and then follow the steps below.

npm i

open node_modules/three/package.json

# Modify the sideEffects field to ["./examples/jsm/nodes/**/*"] (the fix for #27376)

npm run build
npm run preview
hybridherbst commented 11 months ago

For reference

hybridherbst commented 11 months ago

I'm still having issues using the Nodes system in a bundled build; I'll continue to investigate why the sideEffects specified in package.json don't have an effect in my setup. Would be great if this issue could stay open in the meantime.

On r160 I'm getting

FunctionOverloadingNode: FunctionNode must be a layout.

in Proxy.setup every frame during the renderloop, only in bundled builds.

And this once at startup:

image
hybridherbst commented 11 months ago

@CodyJasonBennett @XiSenao to keep you in the loop, I'm still trying to get to the root of this problem.

One thing I found is that locally referenced dependencies seem to behave differently in vite, I opened a new vite issue about that.

sunag commented 11 months ago

@sunag as MaterialX can be loaded at runtime, I think the nodes can't really be treeshaken anyways, right? Couldn't Nodes.js import all of them and thus ensure an order and avoid side-effects?

Importing all nodes should work, I think the nodes should support tree-shaking as soon as we review the method chaining.

Mugen87 commented 4 weeks ago

@hybridherbst After all the refactorings this year, is this still an issue?