three.js icon indicating copy to clipboard operation
three.js copied to clipboard

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

Open hybridherbst opened this issue 1 year ago • 16 comments

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

  • https://stackblitz.com/edit/vitejs-vite-9b37bu

Screenshots

No response

Version

r159

Device

Desktop, Mobile

Browser

Chrome, Safari

OS

Windows, MacOS

hybridherbst avatar Dec 11 '23 22:12 hybridherbst

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)

  • https://github.com/mrdoob/three.js/blob/f394e711c1c685042ec84a6005c99393a5acac20/examples/jsm/nodes/code/FunctionNode.js#L138

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
      },
    },
  };
});

hybridherbst avatar Dec 13 '23 08:12 hybridherbst

Hasn't this been solved in #27189?

marcofugaro avatar Dec 13 '23 09:12 marcofugaro

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 avatar Dec 13 '23 14:12 hybridherbst

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.

hybridherbst avatar Dec 13 '23 17:12 hybridherbst

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.

CodyJasonBennett avatar Dec 14 '23 12:12 CodyJasonBennett

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

XiSenao avatar Dec 14 '23 12:12 XiSenao

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 avatar Dec 14 '23 17:12 hybridherbst

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

@Mugen87 can you reopen this issue please?

hybridherbst avatar Dec 14 '23 17:12 hybridherbst

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.

sunag avatar Dec 14 '23 19:12 sunag

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?

XiSenao avatar Dec 15 '23 01:12 XiSenao

@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)

hybridherbst avatar Dec 15 '23 11:12 hybridherbst

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

XiSenao avatar Dec 15 '23 11:12 XiSenao

For reference

  • https://github.com/mrdoob/three.js/issues/25526

hybridherbst avatar Dec 15 '23 15:12 hybridherbst

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 avatar Dec 22 '23 17:12 hybridherbst

@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.

  • https://github.com/vitejs/vite/issues/15412

hybridherbst avatar Dec 23 '23 00:12 hybridherbst

@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.

sunag avatar Dec 23 '23 21:12 sunag