luma.gl icon indicating copy to clipboard operation
luma.gl copied to clipboard

Version check on init is too strict

Open mitresthen opened this issue 3 years ago • 1 comments

Actual Result

The error message from https://github.com/visgl/luma.gl/blob/master/modules/api/src/init.ts#L19 is thrown.

Expected Result

When using a released version of luma.gl the VERSION check could be expected to allow different patch/bugfix versions as long as they are within the same minor version.

Reproduce Steps

Use luma.gl as part of deck.gl in a component library, which is then integrated into different micro-frontends that are stitched together. The yarn.lock file of the different frontends will have different patch versions of luma depending on when they were last updated, so one of the apps will cause globalThis.luma to be assigned a specific version, and another will perform the check, see that there is a bugfix/patch version mismatch and fail the init. Unless there is a reason for the bugfix/patch version to match, the VERSION could be parsed to semver, and a check for compatible minor versions could be performed instead of an exact VERSION match.

mitresthen avatar Aug 11 '22 12:08 mitresthen

Agree that in principle it should be possible to mix and match minor versions, but this is not something we officially support.

I can mention one serious problem is that each copy (patch version) of luma.gl library will create different versions of the same classes which works most of the time, but e.g. makes instanceof checks fail if objects are interchanged between copies. Having multiple versions of luma.gl/core is a bad thing.

And also offer some perspective:

In vis.gl we try to avoid bumping major version for long times to maximize compatibility which means that a lot of stuff can change under the hood between e.g. v8.0 and v8.5, and making sure that such changes work across all combinations patch releases is not feasible. In fact, we do zero testing for this case, and unless the fix to any issue caused by mixing patch versions is trivial, I am not sure any current contributor would be excited to work on fixing it, when the workaround is just to align patch versions.

Keep in mind that vis.gl is designed as a big suite of monorepos each with composable modules and while we strive to follow the elegant semver principles and there is very substantial and complex work under the hood on solving build and release versioning related issues to make everything work smoothly for user and we can't cover all cases.

Finally, I have to admit that I don't fully understand your multiple yarn.lock file setup. Normally one would use yarn workspaces to bring together projects, resulting in a single yarn.lock, but it sounds like you are stitching together the results with external scripts, so a fairly custom setup?

ibgreen avatar Aug 11 '22 13:08 ibgreen

Thanks for the thorough answer! Yes the setup we have is fairly custom with a specific integration repo containing scripts which pulls, builds, creates docker containers for and runs all the projects , but we are working on changing the project setup in a way which should fix this problem. Will probably use yarn resolutions in the component which imports deck.gl to pin the luma.gl version in the meantime. Considering the need for a specific luma.gl version, I'm wondering why e.g. @deck.gl/core uses ^version of @luma.gl/core instead of depending on a fixed version which has to be manually updated. Would imagine the use of ^version would mean the dependency package has to follow all semver rules.

mitresthen avatar Aug 16 '22 06:08 mitresthen

The issue is not "installing the same version" here, but "installing only one copy". IMO the current check is not strict enough. Any instanceof Buffer check would fail if multiple copies are included. I strongly recommend you force resolve to a single copy of deck/luma in your bundle configuration. Even if things are working, these are not small libraries and including multiple copies will inflate your bundle size unnecessarily.

Pessimistress avatar Aug 16 '22 14:08 Pessimistress