Skip to content

Conversation

@naftalibeder
Copy link
Collaborator

@naftalibeder naftalibeder commented Apr 27, 2022

I've been wanting to do this for a while, because I think it will really speed up and simplify development work. This also changes the camera from a class to a functional component, and brings in modern memoization techniques.

It's a work in progress at the moment, but prop-based updates work perfectly. Just re-implementing the imperative methods. Those are done too.

Checklist

  • Component
  • Types
  • Tests
  • Docs

@naftalibeder naftalibeder marked this pull request as draft April 27, 2022 19:14
@naftalibeder
Copy link
Collaborator Author

@mfazekas I also wanted to check about documentation - the Camera props are typed and /** documented */ in the file, but the generation script doesn't seem to be reading them. I haven't looked in much detail, but any ideas offhand?

@naftalibeder naftalibeder changed the base branch from main to chore/typescript May 18, 2022 19:23
@naftalibeder
Copy link
Collaborator Author

@mfazekas Here's the current status:

What's going well

  • The main project builds all TS and JS into ./lib as part of the generate script. The output is a collection of .js and .d.ts file pairs. All file paths should be the same as before.
  • The main project's package.json references ./lib as the module start location.
  • The example project is set up to reference the parent package more visibly, with the file path .. in example/package.json.
  • Types (not components; see below) are exported and can be correctly referenced.

What's not going well

  • The default export (MapboxGL) Is not currently typed, so all of its properties are any. I haven't fixed this yet because... (see next).
  • The component type declarations are not working quite right. Components that start as TS (Camera.tsx) have correctly typed props, but ones that start as JS (e.g. MapView.js) give the error JSX element class does not support attributes because it does not have a 'props' property.. I'm avoiding errors right now by referencing MapboxGL.MapView (typed as any) instead of the direct typed export.
  • Installing the main project as a package from the example can be a little finicky, so a workaround is to bundle the parent project with npm pack, and then reference that by npm install-ing ../<output-archive>. This behaves exactly as if it were a publicly-available npm package.

So to make this ready for main, we still need:

  1. Fix component type declarations.
  2. Type the default MapboxGL export.

I can't justify continuing to work on this non-stop, but it's really close, and I'm stuck on (1) above. It would be great if I could get a second pair of eyes. @mfazekas if you're okay with it, I'll merge it into a new chore/typescript branch, and then it'll be easier to collaborate on this.

@mfazekas
Copy link
Contributor

@naftalibeder amazing, thank much! Yes feel free to merge it into a chore/typescript branch.

Doing npm pack for example sounds scary for me, we'll need to find a solution for that too.

@naftalibeder
Copy link
Collaborator Author

Doing npm pack for example sounds scary for me

Just to be clear, at least for me, the example works perfectly directly referencing the parent project. But I won’t believe it’s a perfectly stable way of doing it until I see a few more people do it successfully :) I mean the npm pack option just as a backup strategy.

@naftalibeder naftalibeder marked this pull request as ready for review May 19, 2022 12:05
@naftalibeder naftalibeder merged commit 966f954 into rnmapbox:chore/typescript May 19, 2022
@naftalibeder naftalibeder mentioned this pull request May 19, 2022
3 tasks
@naftalibeder naftalibeder deleted the chore/camera branch February 21, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants