[Feat] Simplified peerDependencies policy

[donmccurdy]

Target Use Case

An idea for discussion.

Some modules now have a significant number of peer dependencies. @deck.gl/carto is one example, with seven peer dependencies, soon to be eight, which increases the complexity of setup for new users.

As a maintainer, it's not always clear to me which modules belong in dependencies vs. peerDependencies, or whether there's an established policy. See the @luma.gl/* dependencies here:

deck.gl/modules/aggregation-layers/package.json

Lines 40 to 51 in fa029dd

	"dependencies": {
	"@luma.gl/constants": "^9.0.11",
	"@luma.gl/shadertools": "^9.0.11",
	"@math.gl/web-mercator": "^4.0.0",
	"d3-hexbin": "^0.2.1"
	},
	"peerDependencies": {
	"@deck.gl/core": "^9.0.0-beta",
	"@deck.gl/layers": "^9.0.0-beta",
	"@luma.gl/core": "^9.0.0",
	"@luma.gl/engine": "^9.0.0"
	},

Each time a peer dependency is added, or its version incremented, that is technically a breaking change under semver. Whether it breaks users in practice would depend on whether the package in question was already somewhere in the dependency chain, which is hard to predict.

Proposal

Perhaps a way to simplify this — while avoiding the "mismatched dependencies" risk — would be to take a policy that the relevant */core module is always a peer dependency, while any other visgl-related modules are production dependencies, unless there's some particular reason for an exception?

Because everything else depends on the */core modules, I think that could be a way to minimize what end-users need to install, while also ensuring that compatible versions are used.

[Pessimistress]

Just documenting the reasoning I followed in #8573:

The peer dependencies used in v9.0:

• Packages that will break if multiple copies are installed
  • @deck.gl/core: single-version enforcement; use of instanceof Effect, Viewport in the code base
  • @luma.gl/core: single-version enforcement; use of instanceof Buffer, Texture, Device in the code base; global device registry
  • @luma.gl/engine: I was under the impression that ShaderFactory, PipelineFactory create global singletons but it doesn't appear to be the case?
  • @loaders.gl/core: I was under the impression that it used a global registry but it doesn't appear to be the case?
• Packages that are reused to minimize bundle size (other @deck.gl/*). Our dev-tools setup lists all peerDependencies as externals when building the dist bundle. So that when multiple bundles are used together, they do not include the same dependency twice. The result is that script tag usage mirrors npm usage. Doing so will minimize the combined bundle sizes, should the user choose to selectively include sub-modules with script tags.
• Frameworks that the user will want to manage themselves ("true peer dependency"): react, react-dom, @arcgis/core

Reflecting on the above:

• I agree that the number of peer dependencies should be minimized. Although @deck.gl/core lists some of these as dependencies, you still get warning/error if you don't list them explicitly. The main module currently triggers a wave of warnings because of this.
• I think single-version enforcement is for a good reason. Requiring a single instance, on the other hand, may be excessive, and even troublesome, for example see Export BufferTransform from luma.gl #8726
• We should get rid of the global singletons in general. They don't port well across the various distribution formats and cause hard-to-debug issues when things go wrong.
• It's possible to remove the various instanceof usages but hard to prevent them from creeping back.

[ibgreen]

• luma.gl/core - the device registration could be stored on globalThis. Have also been working on adding a devices: Device[] parameter to luma.createDevice so that global registration is optional.
• luma/engine singletons are per device
• luma.gl/shadertools - the ShaderAssembler singleton is still global. We did however discuss moving away from global registration of shader hooks etc.
• loaders does use globalThis. so sharing works between multiple versions of the same module - the downside is that this could potentially lead to insanely hard to debug issues if two slightly incompatible versions modified the same global.

[ibgreen]

• Requiring a single instance, on the other hand, may be excessive, and even troublesome, for example see Export BufferTransform from luma.gl #8726

  • Yes... I am not sure what aspect of single instance that issue refers to.

• We should get rid of the global singletons in general. They don't port well across the various distribution formats and cause hard-to-debug issues when things go wrong.

  • Agreed.

• It's possible to remove the various instanceof usages but hard to prevent them from creeping back.

  • We do need to check runtime types in various functions. If we can't use the built-in functionality we need to add our own type field or some sniffing mechanism. My hesitation is that the solutions I can imagine are ugly enough that we should make sure it is worth it, that it really makes a difference in usage.

[Pessimistress]

I am not sure what aspect of single instance that issue refers to

#8726 highlights the fragile setup surrounding @luma.gl/core.

• @deck.gl/core bundle exposes its copy of @luma.gl/core via the globalThis.luma. The other @deck.gl/* imports luma core classes from this copy instead of bundling their own, to work around the single-instance requirement.
• If the user wants some luma core exports that are not bundled by deck core, or simply by mistake, they include the @luma.gl/core dist bundle via a script tag. This will practically introduce a second copy of luma core. The classes in globalThis.luma may come from either copy depending on which one is executed first. Now any subsequent @deck.gl/* and @luma.gl/* code may break when they reference instanceof <core_class> or a global singleton.
• To avoid this, we receive requests to export more luma classes from deck core, raising concerns of bloated bundle size.

[ibgreen]

Well, we could always take another look at the state in luma.gl master (targeting v9.1), which removed the non-essential exports (often by moving them into internal functions in non-core modules).

[Pessimistress]

We do need to check runtime types in various functions. If we can't use the built-in functionality we need to add our own type field or some sniffing mechanism

The majority of the usages are in the following pattern:

getAttachment(attachment: string | Texture | TextureView): TextureView {
	if (typeof attachment === 'string') {
		return createTexture(attachment).view;
	} else if (attachment instanceof Texture) {
		return attachment.view;
	} else {
		return attachment;
	}
}

Which can be converted to:

getAttachment(attachment: string | Texture | TextureView): TextureView {
	if (typeof attachment === 'string') {
		return createTexture(attachment).view;
	} else if ('view' in attachment) {
		return attachment.view;
	} else {
		return attachment;
	}
}

TypeScript won't have a problem with this during type check. If in runtime the user passes in some JavaScript object that is neither Texture or TextureView - the current implementation wouldn't work either.

With that said, we don't have an easy way to prevent these from being added back.

[ibgreen]

TypeScript won't have a problem with this during type check

That would be nice. I have never been able to get the in operator to do type narrowing, so I would expect that a cast would needed in both that branch and the last one. But typescript evolves, perhaps I should try again.

Even so this will need custom approach in every place.