-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
Comments
Hey @neodescis - thanks for raising this, sorry to hear it's causing you problems. To my understanding you shouldn't need https://github.com/JamesLMilner/terra-draw-route-snap-mode 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. |
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. |
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. |
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. |
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. |
#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. |
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. |
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! |
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:
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! |
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:
|
@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. |
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. |
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:
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:
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.
The text was updated successfully, but these errors were encountered: