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

Move kdbush to be a dependency instead of dev dependency #2014

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Jan 1, 2023

Launch Checklist

kdbush was introduced in a recent PR.
When using version 3.0.0-pre.2 with typescript it saids that you need to install the kdbush types.
This is not a great expericence.
This is similar to geojson, point, vctor-tiles etc.

image

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

Bundle size report:

Size Change: 0 B
Total Size Before: 208 kB
Total Size After: 208 kB

Output file Before After Change
maplibre-gl.js 198 kB 198 kB 0 B
maplibre-gl.css 9.1 kB 9.1 kB 0 B
ℹ️ View Details No major changes

@rotu
Copy link
Contributor

rotu commented Jan 1, 2023

This is puzzling to me. When is the above message happening?

Is kdbush an externalized dependency? It seems to me like it’s bundled and it (and certainly its types) should be under devDepencencies.

@HarelM
Copy link
Collaborator Author

HarelM commented Jan 1, 2023

The generated types definition is creating the maplibre.d.ts file with import KDBush from 'kdbush'; When using this without installing the types you'll get the following error.
It might be a limitation of the tool we are using to generate the maplibre.d.ts file, IDK...

@rotu
Copy link
Contributor

rotu commented Jan 1, 2023

Two things:

  1. I think we should only export types for exposed interfaces (not sure how hard to make this change, but essentially --export-referenced-types)
  2. In the near-term, we should use --external-inlines kdbush etc. so that the d.ts is self-contained.

@HarelM
Copy link
Collaborator Author

HarelM commented Jan 1, 2023

Feel free to try it out and replace this solution. I don't have a strong feelings about the current solution.
Note that inlining packages definitions might cause issue for people referencing these external libraries, I guess...
In any case, there's a bug that needs to be fixed in the current pre-release...

@rotu
Copy link
Contributor

rotu commented Jan 2, 2023

I don't really know what I'm doing here, but I gave it a whack in #2015.

@HarelM HarelM mentioned this pull request Jan 3, 2023
@wipfli
Copy link
Contributor

wipfli commented Jan 3, 2023

I think we had similar things in the past. If that fixes the issue for now I guess it is fine to merge...

@HarelM HarelM merged commit 3154b26 into main Jan 3, 2023
@HarelM HarelM deleted the fix-missing-types-packages branch January 3, 2023 13:18
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.

4 participants