OpenGeoscience / geojs

High-performance visualization and interactive data exploration of scientific and geospatial location aware datasets
https://opengeoscience.github.io/geojs
Apache License 2.0
436 stars 75 forks source link

geo.lean.js, geo.js, and geo.min.js behave differently at build time #1026

Closed waxlamp closed 5 years ago

waxlamp commented 5 years ago

Disclaimer: I'm on 0.19.4, so things may be different now.

Using geo.lean.js (which is the default option as specified in the package.json file) causes a "couldn't resolve vtk" type of error when building my project. This is annoying but doesn't seem to interrupt the runtime usage of GeoJS. However, by default CircleCI will not allow for deploying a build using this version, because it treats all warnings as errors during deployment.

Locally, using geo.js instead (by explicitly changing the import statement that brings geo into the code) causes an OOM error, though I am not too sure why.

geo.min.js works both locally and on CircleCI, so I'm using that for now.

So this is both a bug report and a question:

Bug Report

The three built JS files behave very differently, and they probably shouldn't.

Question

What is the purpose of the three different bundles? When would I use one or the other, if all three were behaving well?

Thanks!

manthey commented 5 years ago

There are two versions: geo.js and geo.lean.js, both of which have minified versions (geo.min.js and geo.lean.min.js). Conceptually, there are some optional dependencies that geo.js has that are not required but add functionality: hammer.js to add touch support, d3 to add some features to the svg renderer, vtk.js to add the vtkjs renderer. geo.js has all of these bundled into its code. geo.lean.js does not, but if they are available in the global namespace, it can still use them.

There was a bug in how vtk.js was referenced in the lean version (webpack optional dependencies have to be of the form try { var foo = require(foo); } catch (err) {}, but we had a log message in the catch). This was fixed in #1018, but hasn't been released yet, mostly because we haven't actually gone to a true continue release process and were hoping to roll some other updates into the next release (but could emit a release immediately upon request).

I'm marking this resolved (by #1018).

waxlamp commented 5 years ago

Thanks for the update! Continuous release will be a nice thing to have. For now I'm just using the full bundle in my code but when there is a release I'll switch back to the lean bundle.

subdavis commented 3 years ago

I'd like to keep the hammer and d3 deps but get rid of VTK, but I'm not sure what the best approach to this is.

currently I've just removed vtk.js references from yarn.lock and set my geojs dependency to exactly 1.0.0. This works, but it's not very elegant. I don't know if there are any better suggestions for how to exlcude vtk, which is doubling my bundle size.

manthey commented 3 years ago

I suppose we could add another variant: geojs.novtk.js or some better name. Alternately, we could remove vtkjs and always require that it be explicitly added if needed -- I'm not sure who is actually using it. We should probably discuss the options with @aashish24.

aashish24 commented 3 years ago

I would be incline towards removing vtk.js and require adding it afterwards if someone is using geojs-vtk.js interface. Does that answer your question @manthey