mrdoob / three.js

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

`import` fails to resolve when using `esbuild` build tool #24593

Closed wang1212 closed 1 year ago

wang1212 commented 1 year ago

Is your feature request related to a problem? Please describe.

When using the esbuild build tool, the import statement reports an error that cannot be resolved.

E.g:

import { OrbitControls } from 'three/examples/jsm/controls/OrbitControls';

This is because of the following configuration:

"exports": {
  "./examples/jsm/*": "./examples/jsm/*"
}

Although tools like rollup, webpack, etc. may be normal, obviously esbuild respects node specification standards more.

According to the node specification, in order for the esbuild tool to work properly, the file extension needs to be added:

import { OrbitControls } from 'three/examples/jsm/controls/OrbitControls.js';

Obviously when this happens, many people need to spend as much time as I do to analyze the cause of the problem.

In fact, the following configuration can also solve the problem:

"exports": {
  "./examples/jsm/*": "./examples/jsm/*.js" 
}

In order to improve development efficiency (compilation speed), esbuild should have been widely used, can this problem be solved?

see also https://github.com/evanw/esbuild/issues/2518

Describe the solution you'd like

  1. For files of the same type (such as .js), change the exports configuration in package.json, for example
"exports": {
  "./examples/jsm/*": "./examples/jsm/*.js"
}
  1. For different categories of files contained in a folder, multiple configurations can be written, or it is clearly mentioned in the documentation that developers should write extensions when importing?
"exports": {
  "./examples/jsm/*.js": "./examples/jsm/*.js",
  "./examples/jsm/*.png": "./examples/jsm/*.png"
}

Describe alternatives you've considered

Additional context

gkjohnson commented 1 year ago

If you'd like esbuild to be more flexible in its ability to resolve package paths I recommend making an issue or PR to the esbuild project rather than one here. But either way it is not common for packages to export files in the style you have recommended. And as you've mentioned this kind of export can lead to file ambiguity. It's best to explicitly import the files you want to your project. You can see that even react-dom does not export files in a way that affords dropping the file extension.

donmccurdy commented 1 year ago

I thought omitting the .js extension on imports was mostly a TypeScript-ism, is it used outside TS?

I don't think we should be doing that anywhere within this project, including examples...

gkjohnson commented 1 year ago

I thought omitting the .js extension on imports was mostly a TypeScript-ism, is it used outside TS?

Yes... Again see here in react-dom.

If people want to use a unique, non-standard style of file imports like this they'll need to configure their build system to support it. It can't be each packages responsibility to accommodate it.

CodyJasonBennett commented 1 year ago

Explicit file extensions are required since threejs is an ES module. It's not correct to blame other tools for following node resolution, they're working as intended.

LeviPesin commented 1 year ago

I don't like extension-less imports because they are confusing (no one reading the code would be able to understand whether the code imports JS file, TS file, or even a CSS file) and they wouldn't work if there are multiple files with the same names but different extensions.

marcofugaro commented 1 year ago

Esbuild is following the ES Modules specification (not node), which requires the .js extension to be specified when importing.

Other bundlers (like webpack) allow you to omit the .js extension if you want. But that decision relies on the bundler.

wang1212 commented 1 year ago

Everyone seems to have their own reasons, so is the package.json file a configuration file that belongs to a node module? Was this fact deliberately ignored? For the front-end development ecosystem, node is an engineering infrastructure. Does it make sense to talk about this issue aside from the fact that the package.json file is used as the configuration file of the node module?

Does a package.json file make sense when using a CDN to reference dependencies?

For whether node allows to ignore file extensions, please refer to the official documentation here: --experimental-specifier-resolution=node

I noticed that some people gave examples of react-dom configuration, but in actual development, how many developers will refer to react-dom/src/* files? And a lot of sample code of threejs is very commonly used.

Changing the package.json configuration item is just a suggestion, adding FAQ is also an option, the purpose is to improve the development experience of developers, that's all.

donmccurdy commented 1 year ago

@wang1212 the files you are importing have .js extensions on disk. There is nothing in the ES Module specification that says you can omit the .js extension from an import. Some build tools just happen to allow it.

If you prefer to use that style of imports, and are not using a build tool that enables them by default, I think we have to ask you to configure your build tool accordingly. I do not think it's a good idea for packages like three.js to alias those rewrites themselves; we will cause more problems than we solve unfortunately.

CodyJasonBennett commented 1 year ago

Yes... Again see here in react-dom.

If people want to use a unique, non-standard style of file imports like this they'll need to configure their build system to support it. It can't be each packages responsibility to accommodate it.

This isn't correct, and you shouldn't be consulting react internals as to how to configure packages; react is a very complex and involved codebase. react-dom/src/* is not a public entrypoint, they locally expose internals for sharing and inline dependencies at build-time as CJS. See https://unpkg.com/react-dom@18.2.0/package.json. Notice how they alias react-dom/client to react-dom/client.js and react-dom/server to react-dom/server.js, respectively. Those are flat bundles that are intended to be imported without an extension. Three doesn't make use of flat bundles in subpaths (e.g. #23368) but exports three/examples/* as ESM source code and assets. This is very unique to three, I don't recall many packages that ship source code in another folder if not for source maps.

@wang1212 the files you are importing have .js extensions on disk. There is nothing in the ES Module specification that says you can omit the .js extension from an import. Some build tools just happen to allow it.

To clarify, NPM packages follow Node specification and resolution between ESM and CJS, and three ships examples as ESM-only. Some tools may transpile and allow omitting file extensions for interop (CJS doesn't have this restriction). Vite, based on rollup and esbuild, would be the most notable to do this due to its stack and use across frontend, backend, and testing. I'd recommend that for @wang1212 if their project allows it.

If you prefer to use that style of imports, and are not using a build tool that enables them by default, I think we have to ask you to configure your build tool accordingly. I do not think it's a good idea for packages like three.js to alias those rewrites themselves; we will cause more problems than we solve unfortunately.

I see that the proposed alias was actually reverted via https://github.com/mrdoob/three.js/commit/528193fdac968ed7d872a5e5c6f63012b8a89ac9 of r137. Why is that? Is it not worthwhile to support both methods of imports for JS via https://nodejs.org/api/packages.html#package-entry-points? This would also work with #24595.

// Hypothetical but pertinent to #23368
"./addons": "./examples/jsm/index.js",

"./examples/fonts/*": "./examples/fonts/*",

"./addons/*": "./examples/jsm/*",
"./addons/*.js": "./examples/jsm/*.js",
"./examples/*": "./examples/jsm/*",
"./examples/*.js": "./examples/jsm/*.js",

Otherwise, has this since been documented, or is there no need to? I'm not sure if it's a breaking change but a source of confusion for sure.

gkjohnson commented 1 year ago

This isn't correct, and you shouldn't be consulting react internals as to how to configure packages; react is a very complex and involved codebase.

My only point in that post was that there are large packages choosing to omit extensions in their imports and that it requires a specialized build system to support.

CodyJasonBennett commented 1 year ago

I don't understand what you're saying. I'm sure there's been internal dialogue and patch releases around this (noting https://github.com/mrdoob/three.js/commit/528193fdac968ed7d872a5e5c6f63012b8a89ac9, specifically), but surely the above configuration wouldn't be such a maintenance concern. It is my understanding that it is purely beneficial for user-land.

gkjohnson commented 1 year ago

It was a response to https://github.com/mrdoob/three.js/issues/24593#issuecomment-1236168150 asking whether this only happened in TS.

(noting https://github.com/mrdoob/three.js/commit/528193fdac968ed7d872a5e5c6f63012b8a89ac9, specifically)

The extensions were removed within days of being the exports initially being added (and before a release was made) and likely because the previous implementation with .js precluded the ability to import files with the extension. Three.js has always been designed around writing, distributing, and encouraging code that can be run directly in the browser so I don't expect the project would have deliberately been enabling exports that encouraged extension-less paths that required a build process.

There are lots of ways that people to structure and organize their code. If users have a preferred ergonomic way to import files then more power to them - but I don't think it's three.js' job to directly accommodate all of them. Users can configure their build process to support their ergonomic preferences. Otherwise in userland this is inconsequential. Everything still works all the same.

If someone else on the project is interested in getting this changed added that's fine but I don't expect my opinions are all that divergent from the project opinions on the topic.

CodyJasonBennett commented 1 year ago

How is it not strictly correct for a package published on NPM using an NPM-specific package.json file to prevent the most common and painful interop issue that presents a user-land gotcha through a seemingly trivial and supported configuration?

I don't understand why this is reduced to ergonomics; it otherwise makes authoring on top of three, especially a library, very error-prone and confusing in sometimes unfixable ways, especially if you forgo a build tool that would abstract this away. This is why I bother to comment, and one of the concerns of pmndrs/three-stdlib, aside from tree-shaking, both of which I would really like to be able to contribute here and make redundant following #24595.

I want three examples or addons to be easy and safe to use for end-users, particularly for modern stacks. What can I or we do to get there because I don't see an alternative?

marcofugaro commented 1 year ago

I see that the proposed alias was actually reverted via https://github.com/mrdoob/three.js/commit/528193fdac968ed7d872a5e5c6f63012b8a89ac9 of r137. Why is that?

See #23347. Ultimately it was chosen to match the usage in a pure ES Modules environment (threejs.org/examples), while allowing bundlers to optionally allow no .js imports.

Is it not worthwhile to support both methods of imports for JS via https://nodejs.org/api/packages.html#package-entry-points?

This still doesn't work, the .js get matched to the * entry point. Remember that this is an issue just with esbuild, so the node docs might not be relevant here.

LeviPesin commented 1 year ago

Three doesn't make use of flat bundles in subpaths

No, now we have an alias three/nodes, pointing to file three/examples/jsm/nodes/Nodes.js.

donmccurdy commented 1 year ago

... it otherwise makes authoring on top of three, especially a library, very error-prone and confusing in sometimes unfixable ways ...

I'm not sure that I see how hiding the .js extension from imports makes maintaining a library on top of three.js easier?

We already offer and support too many ways of installing dependencies, many of which are fragile. We've just added three/addons/* in addition to three/examples/jsm/*. I do not want to add more import syntax; I would rather focus on fewer methods that we can fully understand, document, and support.

gkjohnson commented 1 year ago

How is it not strictly correct for a package published on NPM using an NPM-specific package.json file to prevent the most common and painful interop issue that presents a user-land gotcha through a seemingly trivial and supported configuration?

If the concern here is that users cannot import three.js examples modules using require in some contexts then that's fine and I encourage you to make an issue so that can be discussed explicitly but that was not mentioned anywhere in the original post and to be blunt acting like removing the requirement of the js extension is an actual solution to the issue is ridiculous. It just serves to obscure the problem in some cases. As it is the suggested change is superficial and purely an ergonomic improvement.

I want three examples or addons to be easy and safe to use for end-users, particularly for modern stacks. What can I or we do to get there because I don't see an alternative?

Please make a new issue that clearly states the problem and provides practical examples of where it's failing. You have to understand that I (and seemingly the the rest of the three.js maintainers) do not use a comparable build process to what you're familiar with and in fact are working towards removing builds as much as possible for ourselves. So I do not know Vite, Next.js, etc. And I only know esbuild and webpack as much as I have to. I personally detest this ecosystem fragmentation and it's exhausting to deal with so I avoid it as much as possible.

A lot of the discussions around this in the past have been plagued by this disconnect in communication so my only point here is that you need to make it easy for us to see and understand the failures and how it's impacting people in practical development because it is very not obvious at least to me, though I do understand that require cannot import the examples. It's not clear to me how frequently or where require-style imports are still being used consistently.

The importability of the examples has improved significantly in this regard over the last couple years so if there are still issues users I think it's worth raising again. I'm otherwise willing to make recommendations or suggest solutions that will hopefully be acceptable to the project.

donmccurdy commented 1 year ago

You have to understand that I (and seemingly the the rest of the three.js maintainers) do not use a comparable build process to what you're familiar with and in fact are working towards removing builds as much as possible for ourselves.

I'll volunteer as a different perspective on this — I strongly prefer ESM-based build tooling myself. I've personally stopped answering questions about installation unless both ESM and a bundler (or Node.js) are involved; the rest of the options feel exhausting for me.

Future versions of three.js-related packages I maintain (e.g. three-pathfinding, three-filmic, three-to-cannon, mikktspace, and gltf-transform) will eventually stop publishing the CommonJS and UMD entrypoints, just publishing ESM instead. Exact timing TBD but probably not this year.

That difference aside, I agree with @gkjohnson that aliasing GLTFLoaderGLTFLoader.js does not solve a problem affecting package maintainers that I'm aware of. If there's a problem to be solved here other than end-users failing to include a ".js" extension when using an ESM-resolution implementation that requires it, I'd love to understand that better.

CodyJasonBennett commented 1 year ago

Is this a desirable or sincere request (collectively)? Really. I have to ask because I'm getting mixed messages here, and I'm not hopeful for resolve in the interim that doesn't just point people to pmndrs/three-stdlib. Perhaps that's an acceptable workaround?

I'd be happy to contribute integration tests or work towards fixing examples' tree-shaking where we can sidestep this entirely with an addons flat bundle (#23368), but I want to make it clear whether the project is accepting of these before investing the time. The latter, I think, is much more agreeable in any case. Maybe this is something for #24595.

gkjohnson commented 1 year ago

@donmccurdy

eventually stop publishing the CommonJS and UMD entrypoints, just publishing ESM instead

I value your optimism 😁 I hope that it looks like we can do that in a year or two but it's hard to believe when there is no broader community drive or effort (specifically on Node's part) to create a path to ESM-only development or support interoperability between commonjs and esm more fluidly... Perhaps projects removing support for commonjs over time the only way to get the all rolling.

@CodyJasonBennett

Is this a desirable or sincere request (collectively)?

I can only speak for myself but if this is still breaking in practical and common use cases for users then it should be addressed. I just can't tell you if it is or when it would be because it's not how I work. Which is why I'm asking for real world examples and explanations. I can support an effort to improve the projects understanding of the problem which hopefully leads to a compelling enough reason to include example cjs files in a future release - I just will not be the one to instigate it. I have had (begrudgingly) to adjust my other projects (three-mesh-bvh, etc) to support a cjs variants for similar requests, as well.

I sympathize with the concern over wasted time, though, but hopefully an initial explainer issue won't require such an extensive amount of time. Perhaps @Mugen87 @mrdoob and @donmccurdy can express some interest here.

I'd be happy to contribute integration tests or work towards fixing examples' tree-shaking where we can sidestep this entirely with an addons flat bundle (https://github.com/mrdoob/three.js/pull/23368)

I think an uber-bundle, tests, and tree-shaking issues are orthogonal to the issue we're discussing. My understanding is that we're just talking about distributing commonjs-compatible "/examples" code

donmccurdy commented 1 year ago

@CodyJasonBennett I do not understand what problems and solutions your comments refer to in this thread. It doesn't seem like we are all talking about GLTFLoaderGLTFLoader.js aliases at this point? 😕

Is this a desirable or sincere request (collectively)?

I can only speak for myself but if this is still breaking in practical and common use cases for users then it should be addressed.

I have no idea what "this" refers to in either sentence above. Please, another thread. 😅🙏

marcofugaro commented 1 year ago

I'd be happy to contribute integration tests or work towards fixing examples' tree-shaking where we can sidestep this entirely with an addons flat bundle (https://github.com/mrdoob/three.js/pull/23368), but I want to make it clear whether the project is accepting of these before investing the time.

Sure, go for it! I don't think anyone here is opposing to any tree-shaking improvements.

donmccurdy commented 1 year ago

I sympathize with the concern over wasted time, though, but hopefully an initial explainer issue won't require such an extensive amount of time. Perhaps @Mugen87 @mrdoob and @donmccurdy can express some interest here.

+1 for an issue to discuss, I think an explainer on the unsolved problems here would be very welcome.

gkjohnson commented 1 year ago

It doesn't seem like we are all talking about GLTFLoader → GLTFLoader.js aliases at this point? 😕 ... I have no idea what "this" refers to in either sentence here. Please, another thread. 😅🙏

An explainer issue would be helpful here but my understanding is that the core of the issue is that using require to import examples does not work in node or some other build environments that may rely on node or node-like behavior. ie const { OrbitControls } = require( 'three/examples/jsm/controls/OrbitControls.js' ) throws an error in node.

However some build processes enable this functionality but respect the explicit package.json export paths (which currently means an extension is required). And users get confused when they suddenly have to start typing a file extension for require snippets even though "require" typically does not require it which is where this aliasing discussion started. The former is the actual problem, though.

This is where my understanding is at, at least.

donmccurdy commented 1 year ago

Hm, I hadn't read that out of the discussion above. The OP does not mention Node.js, other than the resolution algorithm that many web build tools also follow. To be honest I think that we'll be stuck with examples/js/* for years yet, I've seen no signs that Safari will implement Import Maps. That being the case — if we painstakingly maintain separate directories for CJS/UMD and ESM imports, why would we try to support CJS require() through the ESM directory?

Obviously the whole situation is ... not great. But for reasons that do not seem clearly related to requiring the .js extension or not, per the topic of this issue.

donmccurdy commented 1 year ago

On Node.js — I'm finding that most of my ESM-related pain in Node.js these days is related to particular packages rather than the Node.js runtime itself, which seems quite capable of supporting ESM-only development. Examples would be projects like ts-node and tape (among many), both of which have been an obstacle to ESM adoption in Node.js for me. Node.js has done a lot to support interop, arguably too much given the current .cjs/.mjs situation. I do not believe CJS/ESM interop will improve further in Node.js — I believe any further improvement in the status quo will come down to the package ecosystem shifting toward ESM.

mrdoob commented 1 year ago

I would like to get rid of all the CJS code... by the end of the year...

marcofugaro commented 1 year ago

I would like to get rid of all the CJS code... by the end of the year...

I think this goal is doable!

Furthermore next.js (the reason people asked for .cjs) uses ESM by default since a few versions back.

CodyJasonBennett commented 1 year ago

Obviously the whole situation is ... not great. But for reasons that do not seem clearly related to requiring the .js extension or not, per the topic of this issue.

I don't understand why even this is so controversial with examples being ESM-only. I've yet to see a technical reason why, so I'm hesitant to do anything remotely related. Is https://github.com/mrdoob/three.js/issues/24593#issuecomment-1237845979 supposed to clarify this? I don't know, I'm giving up on this one. If a user or author wants to ship example code to modern stacks without this headache, use three-stdlib, not three/examples.

On Node.js — I'm finding that most of my ESM-related pain in Node.js these days is related to particular packages rather than the Node.js runtime itself, which seems quite capable of supporting ESM-only development. Examples would be projects like ts-node and tape (among many), both of which have been an obstacle to ESM adoption in Node.js for me. Node.js has done a lot to support interop, arguably too much given the .cjs/.mjs situation. I do not believe CJS/ESM interop will improve further in Node.js — I believe any further improvement in the status quo will come down to the package ecosystem shifting toward ESM.

I would like to get rid of all the CJS code... by the end of the year...

I think this goal is doable!

Furthermore next.js (the reasone people asked for .cjs) https://github.com/vercel/next.js/pull/29878.

That's ofc your prerogative, OGL does this, and it's a huge relief there. They also only use the exports field and do not employ a build step.

This kills three-stdlib in regards to dual-package hazard since it cannot fork the core. This is particularly problematic for CJS packages that rely on shared globals like React, so this would still be problematic for Next.js specifically. Ideally, people can choose to transpile instead if they must ship CJS, but they miss out on flat bundles and tree-shaking.

This is why I want to contribute those here since this is stuff we've already solved upstream (minus #23372) and removes the need for three-stdlib rather than a transpilation step altogether. A flat bundle addons (#23368), which relies on tree-shaking, is my compromise and why I mention it in this issue. It sidesteps all of the above.

donmccurdy commented 1 year ago

@CodyJasonBennett I really appreciate your work in three-stdlib and your experience with the JS ecosystem, but I have not been able to understand your comments written in this thread. We seem to be talking about different things. As far as I can tell the OP has nothing to do with CJS or ESM or Node.js — it's just a missing file extension in an import path.

I feel like whatever discussion this thread is branching into would probably be most appropriate for ...

... or a new issue if needed. I would also be glad to see the CJS code removed, per the issue above.

CodyJasonBennett commented 1 year ago

@CodyJasonBennett I really appreciate your work in three-stdlib and your experience with the JS ecosystem, but I have not been able to understand any of your comments written in this thread. We seem to be talking about different things. As far as I can tell the OP has nothing to do with CJS or ESM or Node.js — it's just a missing file extension in an import path.

That is an error coming from enforcement of the ES Modules spec via Node resolution since examples are ESM only. Yes, this is just a missing file extension but it is not very trivial to fix this when a tool is involved. This can be solved with configuration in three or incidentally through a CJS flat bundle which three-stdlib provides (for other reasons). You've made it clear that three isn't interested in this configuration, and that there's a preference for strict ES Modules compliance (which is more future-proof with static web/CDN as three goes ESM-only), so this isn't further actionable.

I apologize for convoluting the issue with three-stdlib and a proposal for tree-shaking and addons, I'll continue in #24595.

gkjohnson commented 1 year ago

That is an error coming from enforcement of the ES Modules spec via Node resolution since examples are ESM only. Yes, this is just a missing file extension but it is not very trivial to fix this when a tool is involved.

What does this mean? Ambiguous statements like this are one reason why this hasn't been going anywhere. I want to help but there have been no concrete examples or way to understand what is breaking and when. Tell us what tool. Show when it's breaking and how. Give us a real example. Without anything concrete this just looks like a superficial change because, again, we are not using the same tools and workflow you might be. I understand it might be more complex that it initially looks but you have to help us help you.

I'm not sure how else to put it other than I don't feel like there's being enough effort put into communicating the issue. And if we don't understand the problem then it will just get inadvertently broken in the future.