mrdoob / three.js

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

Stringified DracoLoader body fails to load with vite ``build.keepNames: true`` #24729

Open marwie opened 1 year ago

marwie commented 1 year ago

Describe the bug When using vite.js configured with keepNames: true and glTF transform with draco compression and referencing a local draco_decoder.js at runtime we receive a Uncaught ReferenceError: m is not defined error.

Probably caused by this section in DRACOLoader

To Reproduce

Steps to reproduce are described here with a minimal repo that produces the error here.

Code Please see this repository here

Live example live example

Expected behavior Mesh gets decompressed

Screenshots image

Platform:

donmccurdy commented 1 year ago

Definition of m, injected by esbuild —

var er = Object.defineProperty;
var m = (d,t)=>er(d, "name", {
    value: t,
    configurable: !0
});

In short, esbuild is injecting code into the bundle that defines a function m, which implements the keepNames behavior. And then esbuild injects references to that function elsewhere in the bundle. Because DRACOLoader needs to serialize and transfer the decoder implementation to a Web Worker, we intentionally avoid any reference to the function's scope (which cannot be transferred), but esbuild is injecting scope-dependent functions here.

Unfortunately, this is not so easy to work around. I suppose if all of the transferable code were stored as a string in DRACOLoader.js, esbuild wouldn't be able to process it, and we wouldn't have a problem. Quite an ugly solution but it may be our only one right now.

The Web Worker API is unfortunately designed with web applications in mind, and really doesn't lend itself easily to libraries like three.js, which need to support downstream consumers with unknown build processes. This has been a recurring problem for us over the years.

Newer build tools do make Web Workers more practical, and (in theory) should fix these issues using code that also works without a bundler:

However, I don't know how to make that switch while keeping our examples/js builds working. I hope the examples/js builds will go away within a year or so, in favor of ES Modules in examples/jsm. If there's a workaround possible that would save us from making deeper changes here until that happens, it would be my preference ... I am curious if things work correctly without keepNames, or if that introduces other problems?

/cc @gkjohnson

donmccurdy commented 1 year ago

Do you by any chance have the same problems when using KTX2Loader, without Draco, or is that an easy thing to test? It might be possible for us to attempt the solution offered by modern build tools in KTX2Loader.js sooner, because we don't provide an examples/js build for that file. This would mean doing something like the following, splitting KTX2Loader into two files:

// KTX2Loader.js
const worker = new Worker(
  new URL( 'three/examples/jsm/loaders/KTX2LoaderWorker.js', import.meta.url ), 
  { type: 'module' }
);
// KTX2LoaderWorker.js
// ... define decoder ...
marwie commented 1 year ago

Thanks for the explanation @donmccurdy.

It works without keepNames luckily and I haven't found any new problems since changing that so I think for the time being it is not a problem for us.

Regarding KTX2: I haven't tried it separately yet but can create a test case and report back here

Mugen87 commented 1 year ago

I hope the examples/js builds will go away within a year or so,

When I remember correctly, @mrdoob suggested to remove examples/js at the end of this year (r148).

gkjohnson commented 1 year ago

However, I don't know how to make that switch while keeping our examples/js builds working. I hope the examples/js builds will go away within a year or so, in favor of ES Modules in examples/jsm.

One method for getting the current file location for relative file loading without imports is document.currentScript.src which we could fallback to when using examples/js if we're desperate. Something like this would get us the current file location (currentScript is not defined in modules afaict):

const currFile = document.currentScript ? document.currentScript.src : import.meta.url;

Unfortunately we can't use the import.meta.url token anywhere in the in a non-module file, though. So we'd have to strip that bit out in the rollup conversion or something.

mzvast commented 1 year ago

So anyway to overcome this ?

donmccurdy commented 1 year ago

@mzvast for now i think you cannot use this keepNames option in esbuild, I don't know of another workaround.

Once we started removing examples/js builds (around EOY?) we may have other options here.

mzvast commented 1 year ago

@mzvast for now i think you cannot use this keepNames option in esbuild, I don't know of another workaround.

Once we started removing examples/js builds (around EOY?) we may have other options here.

Thank you. For a work around, in my project I refactor my code from

x.constructor.name === "someName"

to

x instanceof SomeClass

and it's fine :)

Mugen87 commented 1 year ago

However, I don't know how to make that switch while keeping our examples/js builds working.

Now that examples/js is gone, is it possible to revisit this issue?

donmccurdy commented 1 year ago

Yes, exploring some options in https://github.com/mrdoob/three.js/pull/24946 — it updates KTX2Loader but the same options would apply to DRACOLoader.

Anish-Someashwara commented 6 months ago

@mzvast for now i think you cannot use this keepNames option in esbuild, I don't know of another workaround.

Once we started removing examples/js builds (around EOY?) we may have other options here.

@donmccurdy Hey could you please tell how to overcome this issue in webpack, I am facing the same issue in webpack?

donmccurdy commented 6 months ago

@Anish-Someashwara I don't think any issues in Webpack have been described yet in this thread, and there are a lot of different ways to configure Webpack. Perhaps you could create a .ZIP or simple repository that demonstrates the problem?

Anish-Someashwara commented 6 months ago

Thanks @donmccurdy for the response,

Here's the git repository which can help you to reproduce the error, I have added problem description and steps to reproduce the error in readme file.

Please let me know if you found something missing to reproduce the error.

Repository Link

Thanks!

Anish-Someashwara commented 6 months ago

Thanks @donmccurdy for the response,

Here's the git repository which can help you to reproduce the error, I have added problem description and steps to reproduce the error in readme file.

Please let me know if you found something missing to reproduce the error.

Repository Link

Thanks!

Hey @donmccurdy , can you look into this please

donmccurdy commented 6 months ago

@Anish-Someashwara I can't make any guarantees about when I can look into this. Publishing Web Workers in a library, especially with dependencies, is not easy and there are no "correct" ways to do it that all bundlers support. The Web Workers API is not well-suited to use in libraries, we have to use workarounds, and bundler plugins that obfuscate code will break some of those workarounds. If I am eventually able to look into this, the answer is very likely to be that your obfuscation must be disabled.

If you need a quick solution I would advise using Meshopt+Gzip compression, which decodes faster and is easier to package.