Examples: Convert jsm files to use bare three import before npm publish

[gkjohnson]

Related issue: #17482

Description

Adds a "prepublishOnly" npm script that will convert the jsm examples to import from "three" rather than "three.module.js" before publish. The changes will not be committed but will but will be published to NPM.

After publish there will be modified files in git but we can add a postpublish script of something like "git checkout ./examples/jsm/" if desired.

cc @donmccurdy @Mugen87

[donmccurdy]

I like it! :) But let's increase the difficulty level; there are also imports from other examples/jsm packages:

three.js/examples/jsm/loaders/KTX2Loader.js

Lines 24 to 34 in e07eca4

	import {
	read as readKTX,
	KTX2ChannelETC1S,
	KTX2ChannelUASTC,
	KTX2Flags,
	KTX2Model,
	KTX2SupercompressionScheme,
	KTX2Transfer
	} from '../libs/ktx-parse.module.js';
	import { BasisTextureLoader } from './BasisTextureLoader.js';
	import { ZSTDDecoder } from '../libs/zstddec.module.js';

[gkjohnson]

@donmccurdy

Actually those shouldn't be an issue. The purpose of this is to fix the potential for a bundler to resolve "three" to a different file than "build/three.module.js". The other imports do not run this risk and generally importing using relative file paths in the examples is not a problem.

[donmccurdy]

Ah good point!

One other edge case, suppose the user imports both KTX2Loader and BasisTextureLoader:

import { KTX2Loader } from 'three/examples/jsm/loaders/KTX2Loader.js';
import { BasisTextureLoader } from 'three/examples/jsm/loaders/BasisTextureLoader.js';

Because KTX2Loader depends on BasisTexture loader, now the bundler is being asked to resolve both of these paths:

• three/examples/jsm/loaders/BasisTextureLoader.js
• ./BasisTextureLoader.js

But can't see what else that could possibly resolve to, so this sounds safe to me, but wanted to call it out.

[gkjohnson]

Those two paths should be fine. The core of the issue is the discrepancy between the "main" and "module" fields in package.json. The two paths you mention will unambiguously point to the same file.

[donmccurdy]

Sounds good. I'd like to test this with an npm link setup, will report back if no one gets to it before me.

[donmccurdy]

Works for https://gltf-viewer.donmccurdy.com/! (built with Parcel 1.12)

I guess the remaining question would be whether we need to deal with the examples/js/libs and examples/jsm/libs quirks before this change can be made?

[Mugen87]

I guess the remaining question would be whether we need to deal with the examples/js/libs and examples/jsm/libs quirks before this change can be made?

Um, on what quirks are you referring to?

[donmccurdy]

See #17482 (comment). This change doesn't directly break anything, but does change which CDNs these files support (Skypack yes, Unpkg no). Files not packaged as normal ES Modules (e.g. draco_decoder.js) may need changes to be served on Skypack.

[mrdoob]

Files not packaged as normal ES Modules (e.g. draco_decoder.js) may need changes to be served on Skypack.

Hmm, what will happen with those? 🤔

[gkjohnson]

Hmm, what will happen with those? 🤔

See brief discussion here #17482 (comment).

The draco_decoder.js file as produced by emscripten automatically includes require statements for node packages such as "fs" or "path" if it's running there. The way Skypack seems to work is that it converts require statements into static import ... statements so they're available when needed but since "fs" and "path" are not npm packages and are only expected to be conditionally imported in emscripten skypack throws an error when importing them.

I've been doing a bit of work with emscripten recently and from looking at the options it doesn't afford the ability to convert those requires to use the asynchronous "import()" (might be worth making an issue for), which might work better with skypack depending on how they're used.

I just did a quick emscripten test with one of my other projects and it does look like setting the ENVIRONMENT option to use "web,worker" will result in no require statements in the final result which I think is what would work in this case. I'm not sure if there's a reason draco_decoder would need to be built with node.js in mind or why the "web,worker" version of the build wouldn't work in node in the first place (I don't think a file system would be used in it?) but if there isn't one then we could make a request to the draco maintainers to include that emcc option. You can see the current skypack-transformed state of draco_decoder here (notice the two generated imports at the top).

I'm not sure if there are other files in /libs that might have this issue. All of the other vanilla jsm files maintained by the project should work as expected.

[donmccurdy]

I'm not sure if there's a reason draco_decoder would need to be built with node.js in mind ...

The Draco team publishes npm packages (draco3d, draco3dgltf) for node.js support, and those packages don't seem to work on the web. I think it's fine for the version we host in this repository to drop node.js support, and let node.js users import the official npm packages instead.

[mrdoob]

@donmccurdy Maybe that's another thing the prepublishOnly script could do? (Download the node.js version and push that one instead)

[gkjohnson]

Download the node.js version and push that one instead

I don't think we necessarily think we need the node version. The browser version would need to be be rebuilt by the draco team to target something like the "web" and "worker" environments to avoid including the require statements. Normally when bundling packages like this you'll setup something like wepback to have a fallback for "fs" or "path" so everything will build without error knowing the package will correctly detect if it's running in node or not. Skypack is instead trying to import the packages immediately (because browsers have no synchronous approach to dynamic imports) and not giving the file a chance to determine whether it's running in node or not in the first place.

Just looking through the files in the /libs folder the following files have this issue:

fflate.min.js
ammo.wasm.js
basis_transcoder.js
draco_decoder.js
draco_encoder.js
draco_wasm_wrapper.js

fflate is the only package where the problem isn't caused by emscripten. For FFlate, at least, we should be using the existing browser module version of the package (which does not use require) rather than what we have now. I'm not sure where the current version was copied from but it looks like it's maybe out of date compared to what's there now, which should fix this problem. If the emscripten packages cause concern maybe we can start a thread with the Draco / Basis or Emscripten teams to figure something out.

Having said that none of this will break existing projects that are installing three.js via npm and it will fix projects that are having the common.js vs module duplicate import issue. My opinion is that we shouldn't fret too much about these libraries unless they are easily fixable.

[donmccurdy]

I'll work on a proposal to the Draco team, I have some other suggestions about their NPM package anyway. But I do agree with @gkjohnson, I don't particularly want the /libs dependencies to be burdened with Node.js support. Node.js interoperability with ES modules is really messy, but these users can easily bring in the official Draco NPM package. That package does not work on the web at this time.

If the Draco team isn't interested in publishing a build with ES Modules, it shouldn't be difficult to compile it ourselves with custom flags.

[gkjohnson]

It is a bit messy. But really this is only a problem with Skypack -- there aren't any other great options to do what skypack is trying to do because there's no synchronous browser-native equivalent of require(). With node.js now supporting import statements out of the box I'm looking forward this becoming simpler as packages use require less and less. "Top-level await" may make import an easier to swallow require alternative, as well. If emscripten provided an option to use import instead of require that would be the best option imo. But a browser-only version of those files would be good, too.

[donmccurdy]

If emscripten provided an option to use import instead of require...

I think it does - see https://emscripten.org/docs/tools_reference/emcc.html#emcc-o-target. Could rename the .mjs extension afterward. I've been doing some other WASM builds with AssemblyScript (can't use that for Draco unfortunately) at it has pretty nice ES Module output too.

[gkjohnson]

I think it does - see https://emscripten.org/docs/tools_reference/emcc.html#emcc-o-target. Could rename the .mjs extension afterward.

Heh you'd think that would work 😁. From my testing previously setting the output to use the .mjs extension just exports the module with export default Module rather than a UMD wrapper. The 'fs', 'path', etc packages are still imported using require.

Generally I'm pretty impressed with the flexibility and number of options that emscripten has -- it just doesn't seem to be able to do this yet.

[mrdoob]

Not sure I understand what the consensus is here...
Is this PR blocked by wasm based libraries or can it be merged?

[donmccurdy]

Ok, I don't think we need to block this PR on the questions about examples/{js,jsm}/libs. All of those dependencies have a canonical "source of truth" outside this repository, so it's debatable whether we even need to publish them to NPM at all.

[mrdoob]

Alright. Lets do this! 💪

[mrdoob]

Thanks!

[Mugen87]

I think we can close #17482 now?

[mrdoob]

I think so!

[mudroljub]

This change is breaking change for me. I could not import modules from three.js with vanilla JS anymore. For example:

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

Now it is broken, because OrbitControls.js couldn't resolve import three, without Webpack and other build tools. Sadly, it was working with relative imports earlier.

[LeviPesin]

You should not ever write from '/node_modules/something'. Use just import { OrbitControls } from 'three/examples/jsm/controls/OrbitControls.js'.

[mudroljub]

'three/examples/jsm/controls/OrbitControls.js' is not valid link in vanilla JS.
You must give JS a full link to the file.

[LeviPesin]

It is definitely a valid link in Node.js.

[mudroljub]

UPDATE: I found kind a vanilla JS solution by using importmap. See details here:
https://stackoverflow.com/questions/70902863/whats-script-type-importmap-used-for

[ulyssesdotcodes]

importmap does not currently work in workers and it looks like it's not a high priority (WICG/import-maps#2). Having an alternative jsm folder with a relative mapped three import would be extremely useful for workers and for dynamic imports without importmap.

[mrdoob]

Interesting, was not aware of that limitation. But having them in the same repo was confusing for contributors...

Maybe someone can create a new repo for them and run the original script every release?