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

[Bug] Library not usable without skipLibCheck: true? #350

Open
neodescis opened this issue Oct 15, 2024 · 13 comments · Fixed by #351
Open

[Bug] Library not usable without skipLibCheck: true? #350

neodescis opened this issue Oct 15, 2024 · 13 comments · Fixed by #351
Labels
bug Something isn't working

Comments

@neodescis
Copy link

Describe the bug
It seems that I cannot build a TypeScript project that references terra-draw without skipLibCheck enabled in my project. Without that, I get a bunch of TypeScript compilation errors. Here's a sampling:

node_modules/terra-draw/dist/extend.d.ts:1:57 - error TS2307: Cannot find module './adapters/common/base.adapter' or its corresponding type declarations.

1 import { TerraDrawBaseAdapter, BaseAdapterConfig } from "./adapters/common/base.adapter";
node_modules/terra-draw/dist/adapters/google-maps.adapter.d.ts:6:14 - error TS2503: Cannot find namespace 'google'.

6         map: google.maps.Map;
node_modules/terra-draw/dist/adapters/arcgis-maps-sdk.adapter.d.ts:7:27 - error TS2307: Cannot find module '@arcgis/core/layers/GraphicsLayer' or its corresponding type declarations.

7 import GraphicsLayer from "@arcgis/core/layers/GraphicsLayer";

The first error seems to be from referencing a file that isn't provided in the npm bundle (I do not see a common folder, or base.adapter.d.ts anywhere). I'm not sure why that file isn't included?

The other errors appear to come from not having various things installed to support the various adapters. However, I definitely do not want to install OpenLayers and others when I only care about MapLibre. One way something like this is generally solved is to move each adapter into its own npm package to separate out the dependencies.

Terra Draw npm version
1.0.0-beta.6

To Reproduce
Steps to reproduce the behavior:

  1. Create TypeScript project without skipLibCheck: true
  2. Install and import terra-draw
  3. Build

Expected behavior
No build errors!

Additional context
Requiring skipLibCheck: true is onerous, and enabling it would not be doable in my project, where I have numerous internal dependencies that I would definitely like to get type-checked.

@neodescis neodescis added the bug Something isn't working label Oct 15, 2024
@JamesLMilner
Copy link
Owner

Hey @neodescis - thanks for raising this, sorry to hear it's causing you problems. To my understanding you shouldn't need skipLibCheck: true when using Terra Draw. I have just disabled it on two of projects that use Terra Draw directly with 1.0.0-beta.6:

https://github.com/JamesLMilner/terra-draw-route-snap-mode
https://github.com/JamesLMilner/terra-draw-website

And I was not able to see any build errors. Are you able to share the repository with me if it's open source? If not potentially a minimal reproducible example? I would be interested to learn more and want to try and resolve this for you.

@JamesLMilner JamesLMilner added the awaiting feedback Awaiting feedback from a member of the community label Oct 16, 2024
@neodescis
Copy link
Author

neodescis commented Oct 16, 2024

I cannot share my repo, but I was able to recreate it on StackBlitz easily enough. I went with Angular as that's what we're using.

https://stackblitz.com/edit/stackblitz-starters-i6yevv?file=src%2Fapp%2Fapp.component.ts

One thought that jumps to mind is that Angular is shifting to esbuild, which may be a piece to the puzzle. The example above and our project are both using it.

I also went through and verified that all of the tsconfig settings are the same as for our project.

@neodescis
Copy link
Author

neodescis commented Oct 16, 2024

Did this get auto-closed on accident? Your change is good, but the third-party references are still an issue. I've updated the StackBlitz to beta 7.

@JamesLMilner JamesLMilner reopened this Oct 16, 2024
@JamesLMilner
Copy link
Owner

Hey @neodescis, yes it did, if you put an issue in the PR it seems to close it automatically. As you say this initially only resolves the internal issues. Resolving the external dependency issues is harder but I am exploring options for doing that.

@JamesLMilner
Copy link
Owner

JamesLMilner commented Oct 18, 2024

Funnily in trying to reproduce this I actually hit my own issues trying to compile just MapLibre GL with tsc without skipLibCheck. I filed it here maplibre/maplibre-gl-js#4855

Not strictly related to this issue but thought I'd share as has been a bit of a stumbling block for me testing locally.

@JamesLMilner JamesLMilner removed the awaiting feedback Awaiting feedback from a member of the community label Oct 22, 2024
@JamesLMilner
Copy link
Owner

#351 was a red herring and I've closed it.

I'll keep looking into this - it might be tricky to resolve in a nice way unfortunately.

@neodescis
Copy link
Author

neodescis commented Oct 23, 2024

Yes, "optional" dependencies are a difficult thing when TypeScript declarations dependencies come into play. I've seen this solved by separating core functionality and implementation-specific things into their own packages, but that can be rather cumbersome to maintain. I wonder if exposing secondary entry points for the adapters instead would work? It's something I have not tried.

@neodescis
Copy link
Author

Any update here? Is this something I could lend a hand with? I would love to migrate our project from mapbox draw, but not being able to build is a blocker!

@JamesLMilner
Copy link
Owner

Hey @neodescis - sorry I haven't forgotten about this, I was just thinking about the solutions, none of which I have identified I'm super wild about:

  1. Include the mapping libraries at dependencies. This is 'fine' but in an ideal world I'd rather not.

  2. Look at re-exporting the types from the libraries to see if it resolves the problems

  3. Use 'any' for the library imports. Probably my least favourite approach.

I might investigate 2 in a bit more detail and see if it makes sense/works. Option 1 seems like a fallback in the worst case.

If you have any thoughts on how to tackle it I'd be open to hearing them.

Thanks!

@neodescis
Copy link
Author

neodescis commented Nov 26, 2024

Option 1 might appear to work, but it can lead to problems using terra-draw if there is ever a version mismatch between what terra-draw wants and what the consuming project tries to use. Likely both versions will end up in the final bundle, which can obviously be problematic.

I'd be curious if option 2 works; I have not ever tried anything like this.

In my opinion, option 3 should be a definite "no." I would never do that if I were maintaining the repo, as it then becomes much more difficult to deal with breaking changes in the different map libraries.

Really this is what "peer dependencies" were created for, but the idea of optional peer dependencies doesn't work well in TypeScript. I think if I were creating this myself, I would isolate each map-specific adapter into its own npm package. Then, each map-specific npm package can advertise the version of the map library it is intended to work with as a peer dependency. In practice, this does work well, but it can be cumbersome to manage generating and publishing all of the npm packages. Still, it would get you to the state where you only install what you need for your project, and everything is still strongly typed. An example set of packages might be something like this:

  • @terra-draw/core
  • @terra-draw/maplibre
  • @terra-draw/mapbox
  • @terra-draw/ol
  • @terra-draw/leaflet
  • ... etc.

@JamesLMilner
Copy link
Owner

@neodescis I've actually been thinking a lot about the npm organisation + many packages approach. It might actually be the best solution, although the biggest amount of effort. It also helps with other issues like being able to version adapters more easily in isolation, end users not having to worry about unrelated dependencies etc.

I was looking at trying to get to version 1 release over the holiday period, and think this might be the way forward in resolving this problem, but also building sensible foundations for Terra Draw into the future. Others have actually suggested this in the past too but I am increasingly thinking a multi-package approach is the way forward.

@neodescis
Copy link
Author

Would you still leave all of the code in this one repository? I think a monorepo for all of the packages might make a lot of sense, but I'm not familiar with terra-draw's current build and publish process.

@instantgis
Copy link

I have the same problem with Angular 19. Otherwise the library works well. I will need to tweak the styles because I need to show marker symbols with labels.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@JamesLMilner @neodescis @instantgis and others