Typescript integration

[naftalibeder]

See #1872. Copied here:

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.

To do

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

• Fix component type declarations.
• Type the default MapboxGL export.
• Verify local rnmapbox/maps installation (via npm install ..) in the example project.

[naftalibeder]

@mfazekas Everything is working, passes all tests[0], and the types all seem to work. Am I missing anything?!

[0] The docgen failure in the CI test is unrelated.

[mfazekas]

@naftalibeder thanks looks awesome! I'll try to test it tomorrow or the weekend.

[mfazekas]

Sorry I didn't have much time to look into it.

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

[mfazekas]

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

This one we should be able to work around with --watch of tsc compiler. https://www.typescriptlang.org/docs/handbook/compiler-options.html

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

Looking at react-native-reanimated for running the Example they don't use the lib, and also when installing from git directly they don't have a prepare script. They also use babel instead of metro to pick up the library from example (see also #892).

[naftalibeder]

Hey @mfazekas I'm really sorry for the delay! I wasn't sure exactly which direction you wanted to go in, but I took a stab at getting the initial compilation, file watching, and the typescript alias for @rnmapbox/mapsworking based on your comments. What do you think?

[mfazekas]

@naftalibeder sorry I'm a bit behind with this, I could not get babel config working.

I've started #1981 which is the other solution to use babel config to resolve the @rnmapbox/maps similarly by react-native-animated. The watch process works, but if we can avoid that, that's even better, as simplifies dev process.

Note that this PR is way bigger than I'm comfortable with, so I'd like to merge it in pieces. Sorry that it's taking this long and thanks for the patience.

[naftalibeder]

This all sounds good to me! No worries if you want to use this PR more as a template, and not actually merge it. I could see integrating the typescript pipeline first, and then merging the Camera.tsx rewrite separately.

A lot of the complexity of this PR is refactoring the .d.ts files to behave more like index files - putting them in the directories where they describe the contents. If you can find a way to make this work without that big of a change, I’m all for it.

Let me know how I can help or if I’m blocking, but otherwise I’ll just wait to see what you want to do.

[naftalibeder]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

[mfazekas]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

Hi, sorry about this, I've made some progress, I'm still not 100% about how to handle this PR exactly.

So I've made some experiments and it sound like RN can consume typescript projects even if the project is just javascript. I think it's likely because of the react-native babel preset does contains typescript. Sorry I made you implement tsc compile steps, but it looks it's not needed.

Also I've merged #2032 which does implement requestAndroidPermission in typescript.

I've merged #2038 which generates https://github.com/rnmapbox/maps/blob/main/javascript/utils/MapboxStyles.ts, this should eventually be imported from index.d.ts, and replace all mapbox style spec derived stuff from index.d.ts we now maintain manually

The biggest part of this PR is Camera.js => Camera.tsx change, it's a bit hard (or more likely impossible) to review. One thing that would be nice there is if we could keep doing useRef<Camera> instead of useRef<CameraRef>

[naftalibeder]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

That would be really helpful.

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

I've also committed a new component Atmosphere as a TS component, see #2044

[naftalibeder]

See #1872. Copied here:

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.

To do

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

• Fix component type declarations.
• Type the default MapboxGL export.
• Verify local rnmapbox/maps installation (via npm install ..) in the example project.

[naftalibeder]

@mfazekas Everything is working, passes all tests[0], and the types all seem to work. Am I missing anything?!

[0] The docgen failure in the CI test is unrelated.

[mfazekas]

@naftalibeder thanks looks awesome! I'll try to test it tomorrow or the weekend.

[mfazekas]

Sorry I didn't have much time to look into it.

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

[mfazekas]

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

This one we should be able to work around with --watch of tsc compiler. https://www.typescriptlang.org/docs/handbook/compiler-options.html

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

Looking at react-native-reanimated for running the Example they don't use the lib, and also when installing from git directly they don't have a prepare script. They also use babel instead of metro to pick up the library from example (see also #892).

[naftalibeder]

Hey @mfazekas I'm really sorry for the delay! I wasn't sure exactly which direction you wanted to go in, but I took a stab at getting the initial compilation, file watching, and the typescript alias for @rnmapbox/mapsworking based on your comments. What do you think?

[mfazekas]

@naftalibeder sorry I'm a bit behind with this, I could not get babel config working.

I've started #1981 which is the other solution to use babel config to resolve the @rnmapbox/maps similarly by react-native-animated. The watch process works, but if we can avoid that, that's even better, as simplifies dev process.

Note that this PR is way bigger than I'm comfortable with, so I'd like to merge it in pieces. Sorry that it's taking this long and thanks for the patience.

[naftalibeder]

This all sounds good to me! No worries if you want to use this PR more as a template, and not actually merge it. I could see integrating the typescript pipeline first, and then merging the Camera.tsx rewrite separately.

A lot of the complexity of this PR is refactoring the .d.ts files to behave more like index files - putting them in the directories where they describe the contents. If you can find a way to make this work without that big of a change, I’m all for it.

Let me know how I can help or if I’m blocking, but otherwise I’ll just wait to see what you want to do.

[naftalibeder]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

[mfazekas]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

Hi, sorry about this, I've made some progress, I'm still not 100% about how to handle this PR exactly.

So I've made some experiments and it sound like RN can consume typescript projects even if the project is just javascript. I think it's likely because of the react-native babel preset does contains typescript. Sorry I made you implement tsc compile steps, but it looks it's not needed.

Also I've merged #2032 which does implement requestAndroidPermission in typescript.

I've merged #2038 which generates https://github.com/rnmapbox/maps/blob/main/javascript/utils/MapboxStyles.ts, this should eventually be imported from index.d.ts, and replace all mapbox style spec derived stuff from index.d.ts we now maintain manually

The biggest part of this PR is Camera.js => Camera.tsx change, it's a bit hard (or more likely impossible) to review. One thing that would be nice there is if we could keep doing useRef<Camera> instead of useRef<CameraRef>

[naftalibeder]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

That would be really helpful.

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

I've also committed a new component Atmosphere as a TS component, see #2044

[naftalibeder]

See #1872. Copied here:

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.

To do

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

• Fix component type declarations.
• Type the default MapboxGL export.
• Verify local rnmapbox/maps installation (via npm install ..) in the example project.

[naftalibeder]

@mfazekas Everything is working, passes all tests[0], and the types all seem to work. Am I missing anything?!

[0] The docgen failure in the CI test is unrelated.

[mfazekas]

@naftalibeder thanks looks awesome! I'll try to test it tomorrow or the weekend.

[mfazekas]

Sorry I didn't have much time to look into it.

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

[mfazekas]

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

This one we should be able to work around with --watch of tsc compiler. https://www.typescriptlang.org/docs/handbook/compiler-options.html

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

Looking at react-native-reanimated for running the Example they don't use the lib, and also when installing from git directly they don't have a prepare script. They also use babel instead of metro to pick up the library from example (see also #892).

[naftalibeder]

Hey @mfazekas I'm really sorry for the delay! I wasn't sure exactly which direction you wanted to go in, but I took a stab at getting the initial compilation, file watching, and the typescript alias for @rnmapbox/mapsworking based on your comments. What do you think?

[mfazekas]

@naftalibeder sorry I'm a bit behind with this, I could not get babel config working.

I've started #1981 which is the other solution to use babel config to resolve the @rnmapbox/maps similarly by react-native-animated. The watch process works, but if we can avoid that, that's even better, as simplifies dev process.

Note that this PR is way bigger than I'm comfortable with, so I'd like to merge it in pieces. Sorry that it's taking this long and thanks for the patience.

[naftalibeder]

This all sounds good to me! No worries if you want to use this PR more as a template, and not actually merge it. I could see integrating the typescript pipeline first, and then merging the Camera.tsx rewrite separately.

A lot of the complexity of this PR is refactoring the .d.ts files to behave more like index files - putting them in the directories where they describe the contents. If you can find a way to make this work without that big of a change, I’m all for it.

Let me know how I can help or if I’m blocking, but otherwise I’ll just wait to see what you want to do.

[naftalibeder]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

[mfazekas]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

Hi, sorry about this, I've made some progress, I'm still not 100% about how to handle this PR exactly.

So I've made some experiments and it sound like RN can consume typescript projects even if the project is just javascript. I think it's likely because of the react-native babel preset does contains typescript. Sorry I made you implement tsc compile steps, but it looks it's not needed.

Also I've merged #2032 which does implement requestAndroidPermission in typescript.

I've merged #2038 which generates https://github.com/rnmapbox/maps/blob/main/javascript/utils/MapboxStyles.ts, this should eventually be imported from index.d.ts, and replace all mapbox style spec derived stuff from index.d.ts we now maintain manually

The biggest part of this PR is Camera.js => Camera.tsx change, it's a bit hard (or more likely impossible) to review. One thing that would be nice there is if we could keep doing useRef<Camera> instead of useRef<CameraRef>

[naftalibeder]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

That would be really helpful.

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

I've also committed a new component Atmosphere as a TS component, see #2044

[naftalibeder]

See #1872. Copied here:

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.

To do

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

• Fix component type declarations.
• Type the default MapboxGL export.
• Verify local rnmapbox/maps installation (via npm install ..) in the example project.

[naftalibeder]

@mfazekas Everything is working, passes all tests[0], and the types all seem to work. Am I missing anything?!

[0] The docgen failure in the CI test is unrelated.

[mfazekas]

@naftalibeder thanks looks awesome! I'll try to test it tomorrow or the weekend.

[mfazekas]

Sorry I didn't have much time to look into it.

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

[mfazekas]

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

This one we should be able to work around with --watch of tsc compiler. https://www.typescriptlang.org/docs/handbook/compiler-options.html

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

Looking at react-native-reanimated for running the Example they don't use the lib, and also when installing from git directly they don't have a prepare script. They also use babel instead of metro to pick up the library from example (see also #892).

[naftalibeder]

Hey @mfazekas I'm really sorry for the delay! I wasn't sure exactly which direction you wanted to go in, but I took a stab at getting the initial compilation, file watching, and the typescript alias for @rnmapbox/mapsworking based on your comments. What do you think?

[mfazekas]

@naftalibeder sorry I'm a bit behind with this, I could not get babel config working.

I've started #1981 which is the other solution to use babel config to resolve the @rnmapbox/maps similarly by react-native-animated. The watch process works, but if we can avoid that, that's even better, as simplifies dev process.

Note that this PR is way bigger than I'm comfortable with, so I'd like to merge it in pieces. Sorry that it's taking this long and thanks for the patience.

[naftalibeder]

This all sounds good to me! No worries if you want to use this PR more as a template, and not actually merge it. I could see integrating the typescript pipeline first, and then merging the Camera.tsx rewrite separately.

A lot of the complexity of this PR is refactoring the .d.ts files to behave more like index files - putting them in the directories where they describe the contents. If you can find a way to make this work without that big of a change, I’m all for it.

Let me know how I can help or if I’m blocking, but otherwise I’ll just wait to see what you want to do.

[naftalibeder]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

[mfazekas]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

Hi, sorry about this, I've made some progress, I'm still not 100% about how to handle this PR exactly.

So I've made some experiments and it sound like RN can consume typescript projects even if the project is just javascript. I think it's likely because of the react-native babel preset does contains typescript. Sorry I made you implement tsc compile steps, but it looks it's not needed.

Also I've merged #2032 which does implement requestAndroidPermission in typescript.

I've merged #2038 which generates https://github.com/rnmapbox/maps/blob/main/javascript/utils/MapboxStyles.ts, this should eventually be imported from index.d.ts, and replace all mapbox style spec derived stuff from index.d.ts we now maintain manually

The biggest part of this PR is Camera.js => Camera.tsx change, it's a bit hard (or more likely impossible) to review. One thing that would be nice there is if we could keep doing useRef<Camera> instead of useRef<CameraRef>

[naftalibeder]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

That would be really helpful.

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

I've also committed a new component Atmosphere as a TS component, see #2044

[naftalibeder]

See #1872. Copied here:

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.

To do

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

• Fix component type declarations.
• Type the default MapboxGL export.
• Verify local rnmapbox/maps installation (via npm install ..) in the example project.

[naftalibeder]

@mfazekas Everything is working, passes all tests[0], and the types all seem to work. Am I missing anything?!

[0] The docgen failure in the CI test is unrelated.

[mfazekas]

@naftalibeder thanks looks awesome! I'll try to test it tomorrow or the weekend.

[mfazekas]

Sorry I didn't have much time to look into it.

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

[mfazekas]

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

This one we should be able to work around with --watch of tsc compiler. https://www.typescriptlang.org/docs/handbook/compiler-options.html

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

Looking at react-native-reanimated for running the Example they don't use the lib, and also when installing from git directly they don't have a prepare script. They also use babel instead of metro to pick up the library from example (see also #892).

[naftalibeder]

Hey @mfazekas I'm really sorry for the delay! I wasn't sure exactly which direction you wanted to go in, but I took a stab at getting the initial compilation, file watching, and the typescript alias for @rnmapbox/mapsworking based on your comments. What do you think?

[mfazekas]

@naftalibeder sorry I'm a bit behind with this, I could not get babel config working.

I've started #1981 which is the other solution to use babel config to resolve the @rnmapbox/maps similarly by react-native-animated. The watch process works, but if we can avoid that, that's even better, as simplifies dev process.

Note that this PR is way bigger than I'm comfortable with, so I'd like to merge it in pieces. Sorry that it's taking this long and thanks for the patience.

[naftalibeder]

This all sounds good to me! No worries if you want to use this PR more as a template, and not actually merge it. I could see integrating the typescript pipeline first, and then merging the Camera.tsx rewrite separately.

A lot of the complexity of this PR is refactoring the .d.ts files to behave more like index files - putting them in the directories where they describe the contents. If you can find a way to make this work without that big of a change, I’m all for it.

Let me know how I can help or if I’m blocking, but otherwise I’ll just wait to see what you want to do.

[naftalibeder]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

[mfazekas]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

Hi, sorry about this, I've made some progress, I'm still not 100% about how to handle this PR exactly.

So I've made some experiments and it sound like RN can consume typescript projects even if the project is just javascript. I think it's likely because of the react-native babel preset does contains typescript. Sorry I made you implement tsc compile steps, but it looks it's not needed.

Also I've merged #2032 which does implement requestAndroidPermission in typescript.

I've merged #2038 which generates https://github.com/rnmapbox/maps/blob/main/javascript/utils/MapboxStyles.ts, this should eventually be imported from index.d.ts, and replace all mapbox style spec derived stuff from index.d.ts we now maintain manually

The biggest part of this PR is Camera.js => Camera.tsx change, it's a bit hard (or more likely impossible) to review. One thing that would be nice there is if we could keep doing useRef<Camera> instead of useRef<CameraRef>

[naftalibeder]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

That would be really helpful.

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

I've also committed a new component Atmosphere as a TS component, see #2044

[naftalibeder]

See #1872. Copied here:

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.

To do

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

• Fix component type declarations.
• Type the default MapboxGL export.
• Verify local rnmapbox/maps installation (via npm install ..) in the example project.

[naftalibeder]

@mfazekas Everything is working, passes all tests[0], and the types all seem to work. Am I missing anything?!

[0] The docgen failure in the CI test is unrelated.

[mfazekas]

@naftalibeder thanks looks awesome! I'll try to test it tomorrow or the weekend.

[mfazekas]

Sorry I didn't have much time to look into it.

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

[mfazekas]

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

This one we should be able to work around with --watch of tsc compiler. https://www.typescriptlang.org/docs/handbook/compiler-options.html

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

Looking at react-native-reanimated for running the Example they don't use the lib, and also when installing from git directly they don't have a prepare script. They also use babel instead of metro to pick up the library from example (see also #892).

[naftalibeder]

Hey @mfazekas I'm really sorry for the delay! I wasn't sure exactly which direction you wanted to go in, but I took a stab at getting the initial compilation, file watching, and the typescript alias for @rnmapbox/mapsworking based on your comments. What do you think?

[mfazekas]

@naftalibeder sorry I'm a bit behind with this, I could not get babel config working.

I've started #1981 which is the other solution to use babel config to resolve the @rnmapbox/maps similarly by react-native-animated. The watch process works, but if we can avoid that, that's even better, as simplifies dev process.

Note that this PR is way bigger than I'm comfortable with, so I'd like to merge it in pieces. Sorry that it's taking this long and thanks for the patience.

[naftalibeder]

This all sounds good to me! No worries if you want to use this PR more as a template, and not actually merge it. I could see integrating the typescript pipeline first, and then merging the Camera.tsx rewrite separately.

A lot of the complexity of this PR is refactoring the .d.ts files to behave more like index files - putting them in the directories where they describe the contents. If you can find a way to make this work without that big of a change, I’m all for it.

Let me know how I can help or if I’m blocking, but otherwise I’ll just wait to see what you want to do.

[naftalibeder]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

[mfazekas]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

Hi, sorry about this, I've made some progress, I'm still not 100% about how to handle this PR exactly.

So I've made some experiments and it sound like RN can consume typescript projects even if the project is just javascript. I think it's likely because of the react-native babel preset does contains typescript. Sorry I made you implement tsc compile steps, but it looks it's not needed.

Also I've merged #2032 which does implement requestAndroidPermission in typescript.

I've merged #2038 which generates https://github.com/rnmapbox/maps/blob/main/javascript/utils/MapboxStyles.ts, this should eventually be imported from index.d.ts, and replace all mapbox style spec derived stuff from index.d.ts we now maintain manually

The biggest part of this PR is Camera.js => Camera.tsx change, it's a bit hard (or more likely impossible) to review. One thing that would be nice there is if we could keep doing useRef<Camera> instead of useRef<CameraRef>

[naftalibeder]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

That would be really helpful.

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

I've also committed a new component Atmosphere as a TS component, see #2044

[naftalibeder]

See #1872. Copied here:

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.

To do

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

• Fix component type declarations.
• Type the default MapboxGL export.
• Verify local rnmapbox/maps installation (via npm install ..) in the example project.

[naftalibeder]

@mfazekas Everything is working, passes all tests[0], and the types all seem to work. Am I missing anything?!

[0] The docgen failure in the CI test is unrelated.

[mfazekas]

@naftalibeder thanks looks awesome! I'll try to test it tomorrow or the weekend.

[mfazekas]

Sorry I didn't have much time to look into it.

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

[mfazekas]

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

This one we should be able to work around with --watch of tsc compiler. https://www.typescriptlang.org/docs/handbook/compiler-options.html

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

Looking at react-native-reanimated for running the Example they don't use the lib, and also when installing from git directly they don't have a prepare script. They also use babel instead of metro to pick up the library from example (see also #892).

[naftalibeder]

Hey @mfazekas I'm really sorry for the delay! I wasn't sure exactly which direction you wanted to go in, but I took a stab at getting the initial compilation, file watching, and the typescript alias for @rnmapbox/mapsworking based on your comments. What do you think?

[mfazekas]

@naftalibeder sorry I'm a bit behind with this, I could not get babel config working.

I've started #1981 which is the other solution to use babel config to resolve the @rnmapbox/maps similarly by react-native-animated. The watch process works, but if we can avoid that, that's even better, as simplifies dev process.

Note that this PR is way bigger than I'm comfortable with, so I'd like to merge it in pieces. Sorry that it's taking this long and thanks for the patience.

[naftalibeder]

This all sounds good to me! No worries if you want to use this PR more as a template, and not actually merge it. I could see integrating the typescript pipeline first, and then merging the Camera.tsx rewrite separately.

A lot of the complexity of this PR is refactoring the .d.ts files to behave more like index files - putting them in the directories where they describe the contents. If you can find a way to make this work without that big of a change, I’m all for it.

Let me know how I can help or if I’m blocking, but otherwise I’ll just wait to see what you want to do.

[naftalibeder]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

[mfazekas]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

Hi, sorry about this, I've made some progress, I'm still not 100% about how to handle this PR exactly.

So I've made some experiments and it sound like RN can consume typescript projects even if the project is just javascript. I think it's likely because of the react-native babel preset does contains typescript. Sorry I made you implement tsc compile steps, but it looks it's not needed.

Also I've merged #2032 which does implement requestAndroidPermission in typescript.

I've merged #2038 which generates https://github.com/rnmapbox/maps/blob/main/javascript/utils/MapboxStyles.ts, this should eventually be imported from index.d.ts, and replace all mapbox style spec derived stuff from index.d.ts we now maintain manually

The biggest part of this PR is Camera.js => Camera.tsx change, it's a bit hard (or more likely impossible) to review. One thing that would be nice there is if we could keep doing useRef<Camera> instead of useRef<CameraRef>

[naftalibeder]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

That would be really helpful.

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

I've also committed a new component Atmosphere as a TS component, see #2044

[naftalibeder]

See #1872. Copied here:

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.

To do

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

• Fix component type declarations.
• Type the default MapboxGL export.
• Verify local rnmapbox/maps installation (via npm install ..) in the example project.

[naftalibeder]

@mfazekas Everything is working, passes all tests[0], and the types all seem to work. Am I missing anything?!

[0] The docgen failure in the CI test is unrelated.

[mfazekas]

@naftalibeder thanks looks awesome! I'll try to test it tomorrow or the weekend.

[mfazekas]

Sorry I didn't have much time to look into it.

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

[mfazekas]

Regarding example, I could only run example after yarn generate also adding a log statement and to MapView.js and reload did not have any effect until I also ran yarn generate. This is less than ideal and we should avoid if possible.

This one we should be able to work around with --watch of tsc compiler. https://www.typescriptlang.org/docs/handbook/compiler-options.html

The way we run example (metro) was copied from https://github.com/software-mansion/react-native-reanimated, they also seems to be using a mix of .ts and .js and .ts.d files, so maybe we should try to reflect their setup.

Looking at react-native-reanimated for running the Example they don't use the lib, and also when installing from git directly they don't have a prepare script. They also use babel instead of metro to pick up the library from example (see also #892).

[naftalibeder]

Hey @mfazekas I'm really sorry for the delay! I wasn't sure exactly which direction you wanted to go in, but I took a stab at getting the initial compilation, file watching, and the typescript alias for @rnmapbox/mapsworking based on your comments. What do you think?

[mfazekas]

@naftalibeder sorry I'm a bit behind with this, I could not get babel config working.

I've started #1981 which is the other solution to use babel config to resolve the @rnmapbox/maps similarly by react-native-animated. The watch process works, but if we can avoid that, that's even better, as simplifies dev process.

Note that this PR is way bigger than I'm comfortable with, so I'd like to merge it in pieces. Sorry that it's taking this long and thanks for the patience.

[naftalibeder]

This all sounds good to me! No worries if you want to use this PR more as a template, and not actually merge it. I could see integrating the typescript pipeline first, and then merging the Camera.tsx rewrite separately.

A lot of the complexity of this PR is refactoring the .d.ts files to behave more like index files - putting them in the directories where they describe the contents. If you can find a way to make this work without that big of a change, I’m all for it.

Let me know how I can help or if I’m blocking, but otherwise I’ll just wait to see what you want to do.

[naftalibeder]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

[mfazekas]

@mfazekas I wanted to check in about this PR. What are your thoughts about when (or if?) we could start integrating this? I'm happy to bring it up-to-date with main whenever you would be ready.

If you have any concerns that I can address, let me know.

Hi, sorry about this, I've made some progress, I'm still not 100% about how to handle this PR exactly.

So I've made some experiments and it sound like RN can consume typescript projects even if the project is just javascript. I think it's likely because of the react-native babel preset does contains typescript. Sorry I made you implement tsc compile steps, but it looks it's not needed.

Also I've merged #2032 which does implement requestAndroidPermission in typescript.

I've merged #2038 which generates https://github.com/rnmapbox/maps/blob/main/javascript/utils/MapboxStyles.ts, this should eventually be imported from index.d.ts, and replace all mapbox style spec derived stuff from index.d.ts we now maintain manually

The biggest part of this PR is Camera.js => Camera.tsx change, it's a bit hard (or more likely impossible) to review. One thing that would be nice there is if we could keep doing useRef<Camera> instead of useRef<CameraRef>

[naftalibeder]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

What's your thought - should I try incorporating Camera.tsx directly on top of main (i.e. discarding any other compile-related changes I made in this PR)?

That would be really helpful.

[mfazekas]

Great! I did find getting components to work with TS to be particularly challenging, so I'm not yet sure that the TS setup you did in those other commits is sufficient for integrating Camera.tsx.

I've also committed a new component Atmosphere as a TS component, see #2044