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

package.json: Don't export examples/fonts directory #26048

Closed
wants to merge 1 commit into from

Conversation

LeviPesin
Copy link
Contributor

Related issue: #20019 (comment)

Description

I don't really understand why we need it...

@legobeat
Copy link

legobeat commented May 13, 2023

If there is demand for keeping it (I wouldn't know), might be worth republishing under a separate package - not sure how trivial that is with current instrumentation scripts, though.

@github-actions
Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
642.6 kB (159.1 kB) 642.6 kB (159.1 kB) +0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
432.4 kB (104.7 kB) 432.4 kB (104.7 kB) +0 B

@Mugen87
Copy link
Collaborator

Mugen87 commented May 13, 2023

Not including fonts in the packages produced a runtime error with certain build/packing tools in the past. I believe the reason for this is that earlier versions of TextGeometry and related logic had some sort of dependency to a default font (helvetiker).

I think fonts can be removed but it would be good if @mrdoob or @WestLangley could double check this.

@marcofugaro
Copy link
Contributor

We can't remove the fonts folder, people are using it.

See #23354

@mrdoob mrdoob closed this May 13, 2023
@legobeat
Copy link

legobeat commented May 13, 2023

Also I question the motivation of this change in general. I personally don't think the size of the npm package is a real issue. Definitely not something that justifies such potential major breaking changes.

Apart from the aggregate cost of it all (est ~2TB post-extraction through NPM daily), there are scenarios where constraints make size become a real issue and potential blocker. It is, of course, valid for three.js to decide that catering to such needs is not worth the costs involved in such breaking changes - either at this point, or for the likely future. As I didn't see public discussion on the issue for a few years, I figured the response to #26049 would help (potential/future) users with package size considerations manage expectations.

Sorry, but I see no need to invest time and budget into this issue. Let's focus on other topics.

So between that, and the closing without further discussion of these to PRs, I guess we now have much more clarity on the upcoming few years of three.js so the issue won't have to be reraised for a while.

Definitely not something that justifies such potential major breaking changes.

We can't remove the fonts folder, people are using it.

Agreed this would be a breaking change that requires communication to users. What I'd suggest might be something along the lines of for example:

  1. Decide path for splitting out to new package (e.g. fonts/).
  2. Split out this part only and release as identical package under new name (e.g. @three.js/fonts)
  3. In an upcoming release, note the breaking out of this package and tell users that importing from three.js/examples/fonts is now considered deprecated and will be removed in a future version
  4. (Optional) Release version that logs console warning on require/import of legacy path three.js/examples/fonts
  5. After enough time has passed, fully remove the path in a future major. Probably pre-announced and bundled together with whatever other breaking change(s) may be pending

Just saying it's possible to do in a way that has minimal impact on users - eating the cake and having it. It might be useful to know if something like this might ever be considered at some point. But again (and I hope it goes without saying), "not in scope, sort out your own builds" is a valid response and for now I guess this is put to rest. I hope this and the initial buzz isn't considered noise. Thank you for listening to my ted talk.

@legobeat
Copy link

legobeat commented May 13, 2023

Another possible option would be to keep the interface and files of three.js package (and if desired repo source-paths) as-is, and publish the same code but split into new separate modules. The three.js package could then be a minimal proxy/re-export of the component modules, which it contains as dependencies. This would allow a modular approach where users may pick and choose from subpackages, without breaking anything for existing users or those who want an all-batteries-included package. That would look different from these closed PRs and the main change would be in packaging/publishing.

@LeviPesin LeviPesin deleted the patch-4 branch May 14, 2023 07:39
@mrdoob
Copy link
Owner

mrdoob commented May 17, 2023

@legobeat

I have a hard time reading your posts / writing style.

I've not read them and instead I've asked chatgpt to summarize:

The discussion revolves around concerns over the size of the three.js NPM package and the potential reorganization of the 'fonts' folder. Although some argue the package size could be an issue in certain scenarios, there's consensus that these changes aren't necessary now and could potentially cause disruption. A procedure for gradually moving the 'fonts' folder to a new package is suggested, but for now, the focus is to address other issues.

Yes, I agree with that.

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.

5 participants