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

TS: Add separate export sub-path for OpenLayers integration (Proof-of-Concept) #340

Closed
wants to merge 1 commit into from

Conversation

ibgreen
Copy link
Contributor

@ibgreen ibgreen commented Jan 10, 2024

Hi @bjornharrtell,

I am a big flatgeobuf fan and I am also maintaining the @loaders.gl/flatgeobuf module. As its usage is ramping up we have been running into a bunch of mostly bundling related problems, and we are now in the process of forking the flatgeobuf TS source code into loaders.gl (instead of importing the published module) so that we can fix these.

In parallel, I thought I would open some PRs here to see if there is a reasonably smooth path to upstreaming changes. I am starting with a relatively small change in this PR to "test the waters".

And no worries if this is too much to review or deal with right now, as mentioned we can always fork until the situation improves.


Description: Some TS applications are not using openlayers, so it would be helpful to isolate openlayers dependencies and code to a use a sub-path import. This lets flatgeobuf follow the principle that a data loading library should not include UI code by default.

import {...<non-ol exports>} from 'flatgeobuf';
import {...<ol-dependent exports>} from 'flatgeobuf/ol';

As written this will be a breaking change for OL users (they will need to update their import clauses'). If that is not acceptable we could add more export subpaths allowing apps to cherry-pick non-OL code, and keep the root export unchanged.

Note: This PR also moves all OL related files under the ol folder to emphasize the separation.

@bjornharrtell
Copy link
Member

I'm not sure I understand why this is a problem. I would expect importing selectively from a single module would eliminate non imported parts. Then again I have not really investigated it...

I think would like to understand this a bit better before considering the structural change. I don't particularly like having to do it but if there is no other way it might be sensible.

@ibgreen
Copy link
Contributor Author

ibgreen commented Jan 10, 2024

The biggest advantage of explicit export sections in package.json is that we can define separate mjs and cjs entry points (import vs require) for each sub file.

Right now it is hard to use flatgeobuf from cjs applications as it only offers mjs. We can easily add back a CJS dist, but having multiple dists (mjs / cjs) becomes particularly complex when asking apps to import subpaths like flatgeobuf does, as it forces the app to use ugly paths to specify which dist it uses and also risks mixing dists.

With "exports" section infrastructure in place we can add both mjs and cjs exports for each subpath, the bundler will automatically pick the right one based on the type field in the app's package json.

This is a great library but it should be easy to use, and the import and bundling issues are causing problems.

As an example, before forking, loaders.gl has been using fragile looking imports like

import * as geojson from 'flatgeobuf/lib/mjs/geojson.js';
import * as generic from 'flatgeobuf/lib/mjs/generic.js';
import {parseProperties as parsePropertiesBinary} from 'flatgeobuf/lib/mjs/generic/feature';

@bjornharrtell
Copy link
Member

My experience with supporting both cjs and mjs has not been pleasant (in other projects). I see cjs as a thing of the past not needed for anything else than pretty old legacy support. What's your use case for cjs?

In any case, when using mjs I still am not convinced a module split is required to decouple dependencies. Also I'm not keen on making a breaking change. I'm open to look into it further though.

@bjornharrtell
Copy link
Member

I've tested with a basic Vue 3 project with TypeScript to add flatgeobuf dependency and import geojson parts. As far as I can see that works fine without any dependency on OpenLayers. See https://github.com/flatgeobuf/flatgeobuf-vue.

@ibgreen
Copy link
Contributor Author

ibgreen commented Jan 11, 2024

Thanks for engaging. First, some quick thoughts on your comments.

CJS not needed for anything else than pretty old legacy support. What's your use case for cjs?

Supporting CJS is valuable for big existing applications. Adopting mjs in existing apps requires updating toolchains to latest esbuild, vite, etc, which also usually triggers a lot of other updates, e.g. node versions, library versions etc and can require months of work to complete.

I personally work with two large geospatial code bases where we have still have blockers to MJS adoption, but we have been wanting to use your library for a long time.

My experience with supporting both cjs and mjs has not been pleasant (in other projects).

It certainly can be very painful. But in deck.gl / loaders.gl we now just run esbuild on the mjs distribution as the last step, and create an index.cjs file. And then we set the "exports.require" field to that dist. Simple and has worked well for users.

I've tested with a basic Vue 3 project with TypeScript to add flatgeobuf dependency and import geojson parts. As far as I can see that works fine without any dependency on OpenLayers

Thanks. I should to check how that example it bundles and how the generated bundle contains any related code etc.

But the problem I have is that the import of the open-layers functionality is visible in the top level index file (the file also exports 'geojson'). OL code may be "tree shaken out" or otherwise stripped by the particular bundler you are using, or it may be bundled as dead code and still work, but in my view, such UI specific code simply shouldn't be imported as part of the flatgeobuf index file in non-OL apps.

@ibgreen
Copy link
Contributor Author

ibgreen commented Jan 11, 2024

Also I'm not keen on making a breaking change. I'm open to look into it further though.

I believe we can do this without breaking anything. We can keep the existing "main"/"module" fields for any old bundlers that don't support "exports", and we can have have the root path still include open layers, and let apps that worry about that use the subpaths.

I'd be willing to do a couple of additional iterations assuming you are positive to this, but if you aren't keen that is fine, we can stop here for now.

CJS import of flatgeobuf now works in @loaders.gl/flatgeobuf@4.1.0-alpha.8 (forking your code, transpiling it to CJS and using "exports" section in package.json as discussed above), so if any of your users have issues with MJS you can always suggest they try the loaders.gl FlatgeobufLoader until they upgrade their toolchain.

@bjornharrtell
Copy link
Member

If we can provide cjs support without breaking anything for existing usage I think it's worth pursuing that for the official npm package. Your efforts are appreciated. :)

@ibgreen
Copy link
Contributor Author

ibgreen commented Feb 19, 2024

Closing for now. Will reopen if I find the time to make another attempt.

@ibgreen ibgreen closed this Feb 19, 2024
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.

None yet

2 participants