-
Notifications
You must be signed in to change notification settings - Fork 48
Chore: import geojson types directly, do not re-export them #450
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
base: master
Are you sure you want to change the base?
Conversation
c16a9fc to
86a208b
Compare
ibgreen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the intent here.
I do have some concerns, putting on the hat of the vis.gl TSC (Technical Steering Committee) so to speak:
- We are making a minor release, and removing these exports would break apps.
- I am not sure what percentage of users would be affected
- But this is the most downloaded module in deck.gl-community, so it will likely cause some upgrade pains.
- Since the cycle between major releases is quite long, we do occasionally make small breaking changes in the API when we feel it is justified / no good alternative.
- But in this case it is more of a cleanup than a necessary change.
- While the work required to upgrade is small, we don't want to underestimate the inconvenience for users.
An alternative is that we move these definitions to the end of the index.ts and mark them as deprecated, and will be removed in next major release.
- We could also add the
@deprecatedkeyword so that vscode strikes over the symbols.
index.ts:
// DEPRECATED EXPORTS - Will be removed in v10
import type {Point as GeoJsonPoint, ...} from 'geojson';
/** @deprecated Will be removed, import directly from 'geojson' module */
export type Point= GeoJsonPoint;|
Apart from the breaking change concern, I am just providing some perspective in case it could be helpful: As we go down the track of reviewing the types in editable-layers, I think it could be worthwhile to discuss whether we keep the
Overall, I feel that our focus should be on making the editable-layers API fully typed.
Another thing we might possibly want to offer is a small zod schema for geojson types to help validate that input actually matches the types.
|
|
@timnyborg Any thoughts, maybe if you can articulate why you suggested this change we can get to a consensus? |
Honestly, just to clean up imports & exports that were made redundant by my turf 7 upgrade. Turf vs nebula vs geojson type differences gave me no end of trouble over the years, so I see value in settling on the dominant library (geojson) and not reinventing the wheel. That said, I probably won't be using these libraries again for the foreseeable 😢. I've been ensuring all the fixes I submitted to nebula.gl over the years make it in, and they finally have, so I'm not bothered either way. |
Cleans up imports of geojson types, so all files import from
geojsondirectly.Removes export of standard geojson types from the library. This will require anyone doing an
import { FeatureCollection, ... } from '@deck.gl-community/editable-layers'to follow suit.