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

fix: changed index.d.ts imports to reference typescript definitions instead of ts files #2529

Closed
wants to merge 7 commits into from

Conversation

mattijsf
Copy link
Member

@mattijsf mattijsf commented Jan 6, 2023

Fixes #2528

@mattijsf mattijsf requested a review from mfazekas January 6, 2023 09:51
@mattijsf mattijsf changed the title fix: changed index.d.ts imports to reference typescript definitions innstead of ts files fix: changed index.d.ts imports to reference typescript definitions instead of ts files Jan 6, 2023
@mfazekas
Copy link
Contributor

mfazekas commented Jan 6, 2023

@mattijsf thanks much. Does our the example app still works without the lib folder? For development experience on rnmapbox/maps not for users of the library.

@mattijsf
Copy link
Member Author

mattijsf commented Jan 6, 2023

Ah that is a good one. I guess it might not without running bob build first...

@mattijsf
Copy link
Member Author

mattijsf commented Jan 6, 2023

This requires more work I'm afraid. Good to have these checks in place!

@mattijsf
Copy link
Member Author

mattijsf commented Jan 6, 2023

This change causes the type checking to happen on the actual generated definition files instead of the inferred javascript types which is generating some extra errors. I'm trying to fix those but it not so easy in some cases.

For example. Here and there we tend to use [number, number] to indicate x,y coordinates. However we also directly assign for example selectedFeature.geometry.coordinates (an imported type from the geojson lib) to one of these props. Which is types as number[]. These (and others) now cause some conflicts.


return (
<PointAnnotation
id={id}
coordinate={coordinate}
title={title}
draggable
onDrag={(feature: Feature<Point, { id: string }>) =>
Copy link
Member Author

@mattijsf mattijsf Jan 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The id currently not sent by PointAnnotationManager so I removed it from the typescript definitions.

coordinate: [number, number];
coordinate: number[];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the tuple typing in theory but in practice libs like geojson offer points as number[] and these can't be assigned to [number, number] so it ends up being more of a burden.

@mattijsf
Copy link
Member Author

mattijsf commented Jan 6, 2023

@mattijsf thanks much. Does our the example app still works without the lib folder? For development experience on rnmapbox/maps not for users of the library.

For me it worked fine after the above changes within the example project because the prepare script already generates the lib folder.

@mfazekas
Copy link
Contributor

mfazekas commented Jan 9, 2023

For me it worked fine after the above changes within the example project because the prepare script already generates the lib folder.

Yep but switching git branches, etc. doesn't rerun the prepare script AFAIK. We could start a watch process, but it means the developer experience became bit more difficult.

@mattijsf
Copy link
Member Author

mattijsf commented Jan 9, 2023

Ah yes that could be causing some annoyances. I'm not sure on how to solve that problem properly except for migrating all javascript files to typescript more quickly but that is not feasible either.

Ideas?

# Conflicts:
#	index.d.ts
@mfazekas
Copy link
Contributor

mfazekas commented Mar 7, 2023

Ah yes that could be causing some annoyances. I'm not sure on how to solve that problem properly except for migrating all javascript files to typescript more quickly but that is not feasible either.

Ideas?

With #2663, we should be really good with js => ts conversion.

Then what remains are those:

  • RasterDemSource
  • RasterSource.js
  • ImageSource
  • UserLocation.js
  • Callout.js
  • Annotation.js
  • offline/*.js
  • snapshot/*.js

so we're close to removing index.d.ts

@mattijsf
Copy link
Member Author

mattijsf commented Mar 7, 2023

Looks like good progress! I think javascript/classes/* are missing from that list.

Actually, I started migrating some files some time ago here: https://github.com/mattijsf/rnmapbox-maps/tree/typescript-migration
Including the ones from javascript/classes/* but I reverted those in the end due to some difficulties typing the animations correctly.

I did do geoUtils, offline/*.js, snapshot/*.js so feel free to check / cherry-pick those.

@mattijsf
Copy link
Member Author

mattijsf commented Mar 8, 2023

I think we can close this PR in favor of the gradual migration of JS > TS and no longer needing index.d.ts. Which should ultimately fix #2528

@mfazekas
Copy link
Contributor

mfazekas commented Mar 9, 2023

I think we can close this PR in favor of the gradual migration of JS > TS and no longer needing index.d.ts. Which should ultimately fix #2528

I've merged #2671 so we should be good.

@mfazekas mfazekas closed this Mar 9, 2023
@mattijsf mattijsf deleted the typescript-fix branch May 23, 2023 06:33
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.

[Bug]: Multiple typescript errors occur when project specific tsconfig contain stricter rules
2 participants