Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Examples: Inline small utility methods for examples/js #23167

Closed

Conversation

donmccurdy
Copy link
Collaborator

Currently there appear to be two choices for files in examples/{js,jsm} that depend on utilities like BufferGeometryUtils or WorkerPool:

  • (A) Copy/paste the necessary methods
    • Duplicates the code; harder to maintain.
  • (B) import * as BufferGeometryUtils
    • Results in larger bundles for ESM users, and requires CJS/UMD users to manually import the utils file.

I think we could improve things a bit by allowing Rollup to inline methods from certain utility files when it compiles the examples/js versions. This means ESM users get strictly smaller bundles, and CJS/UMD users will have a better developer experience and usually get smaller bundles, although some duplication is still possible there if loading multiple files that depend on the same utilities. Particularly for the ESM benefits I think that's a good tradeoff.

The main side-effect here is related to enabling treeshake: 'recommended'. Rollup tree-shakes a fair bit of dead code from existing examples, but I can't think of any downside to that.

@marcofugaro
Copy link
Contributor

marcofugaro commented Jan 7, 2022

import * as BufferGeometryUtils
Results in larger bundles for ESM users,

What do you mean?

import * as BufferGeometryUtils from 'three/examples/jsm/utils/BufferGeometryUtils.js'

console.log(BufferGeometryUtils.mergeVertices)

is transformed into

function mergeVertices() {
  // ...
}

console.log(mergeVertices);

by any bundler, such as rollup with default build settings.

@donmccurdy
Copy link
Collaborator Author

Hm, BufferGeometryUtils is not being tree-shaken with import * as BufferGeometryUtils, but apparently not for the reason I thought. This check prevents it:

if ( BufferGeometryUtils === undefined ) {
throw new Error( 'THREE.SimplifyModifier relies on BufferGeometryUtils' );
}

@marcofugaro
Copy link
Contributor

oh yeah, that check does not make sense in the modules world.

I think we can remove it, just like for WorkerPool in KTX2Loader, the error is enough to warn the user.

@donmccurdy
Copy link
Collaborator Author

We could, but can you say more about why? Does having these functions inlined in examples/js seem like a problem?

@marcofugaro
Copy link
Contributor

marcofugaro commented Jan 8, 2022

We could, but can you say more about why?

I was talking about examples/jsm these functions are unnecessary because a module will always be defined. Sorry I did not see you already removed them in this PR.

Regarding examples/js, tested your PR, encountered no problem at all!
Just wanted to clarify that using import * as BufferGeometryUtils does not impact tree-shaking at all.

@donmccurdy
Copy link
Collaborator Author

Based on #23175, it sounds like this PR is micro-optimization in an area where larger changes would be better. Closing!

@donmccurdy donmccurdy closed this Jan 30, 2022
@donmccurdy donmccurdy deleted the feat-inline-buffergeometryutils branch January 30, 2022 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants