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

feat(dev-tools): Inline ESM-only dependency #460

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

donmccurdy
Copy link

For discussion.

As of v9, deck.gl provides both ESM and CJS entrypoints. However, some dependencies — notably @mapbox/tiny-sdf — are ESM-only and do not include CJS entrypoints. This seems to be a problem in certain environments as noted in visgl/deck.gl#7735, which may require CJS versions of dependencies.

This PR implements a possible solution in which the @mapbox/tiny-sdf dependency is inlined into the compiled CJS bundle. Optionally, we could do the same for the ESM bundle, and move the package from dependencies to devDependencies. The module is small (1.2kB), and can be tree-shaken, so the inlining seems acceptable.

I assume that if we decide to take this approach, we'd change the API such that the package name is not hard coded into ocular-dev-tools, and we instead either pass a list of package names in the project's ocular config, or attempt to automatically detect which dependencies do not offer CJS entrypoints.

Alternatives considered:

  • Replace @mapbox/tiny-sdf with an alternative
  • Fork @mapbox/tiny-sdf, add CJS build
  • Drop CJS support in deck.gl

@ibgreen
Copy link
Collaborator

ibgreen commented Apr 11, 2024

This may be a useful capability to have in ocular.

That said, my general view is that we should be very selective with our use of dependencies, and for small things, we can just fork the code if the original publishers are no longer active.

In this case it looks like one 140-line code file that has only had one or two micro fixes in the last two years, so why not just copy it into our repo, instead of fighting the JS build systems.

https://github.com/mapbox/tiny-sdf/blob/main/index.js

@donmccurdy
Copy link
Author

donmccurdy commented Apr 11, 2024

I'm open to that! It is indeed a small package. My experience over the past 1-2 years has been more and more packages going ESM-only intentionally, so I don't think the issue is specific to unmaintained dependencies, but no matter.

I use Microbundle for building most of my personal repositories, which has the behavior of automatically inlining any dependency that's found in devDependencies, but missing from dependencies or peerDependencies. I enjoy the feature, but adding the same behavior to Ocular might be overkill for this small issue. :)

@ibgreen
Copy link
Collaborator

ibgreen commented Apr 11, 2024

My experience over the past 1-2 years has been more and more packages going ESM-only intentionally, so I don't think the issue is specific to unmaintained dependencies, but no matter.

That is a good point. A question then is how long should deck.gl be trying to stem that tide?

I use Microbundle for building most of my personal repositories

Yes, in JS there is always another tool... I spent a lot of time learning webpack and now esbuild, so from that perspective I am slightly wary of making another switch so soon, and like the idea of staying with major tools (maybe microbundle is bigger than I think).

@donmccurdy
Copy link
Author

donmccurdy commented Apr 11, 2024

That is a good point. A question then is how long should deck.gl be trying to stem that tide?

I suppose we'll know when we know. 😆

...like the idea of staying with major tools (maybe microbundle is bigger than I think).

Not particularly, I'd say it's a highly opinionated wrapper around Rollup, in the same way that Ocular is (now) a highly opinionated wrapper around ESBuild. Definitely not suggesting a switch in tools, Microbundle is probably not right for all of VisGL ... just helpful to compare default behaviors!


Suggestion: Let's wait to hear if TinySDF remains problematic in v9. I suspect it will, but no point taking action until we're sure. Then we can either fork it or inline it.

return {
name: 'inline-esm-only',
setup(build) {
// TODO: Detect ESM-only from package.json, instead of hard-coding package names?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is difficult to do, can we make it a ocularrc config instead of hard-coding it here?

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.

3 participants