split out compiled js from typescript types and autogenerate both

[natew]

trafficstars

Fixes https://github.com/rnmapbox/maps/issues/2140#issuecomment-1230278406

[naftalibeder]

@natew Thanks for doing this and I support the overall direction of compiling as part of the publish step.

I'll leave it to @mfazekas to decide whether this is the direction the library can go, but a couple thoughts:

1. Shouldn't the prepare script include compiling/packaging? It gets run on npm install, so without that, it'll be hard to install the library directly from Github, or from a local copy.
2. It would be nice if the declaration files got generated directly into lib, so they didn't clutter up the source project.

Edit: https://github.com/callstack/react-native-paper is a good example.

[mfazekas]

@natew thanks for the PR, but we still wait for repro steps for the issue you're seeing.

Metro bundler does transpile packages in node_modules so publishing transpired stuff is optional.

Also following should be checked/verified:

• [ ] our example app works just fine (and editing package file and pressing r should resuling in example with modified code. Running a tsc watch is possible but not ideal.)
• [ ] use package with yarn add rnmapbox/maps#main and npm install rnmapbox/maps#main works

BTW if you look at react-native-reanimated they also point to src/index and not to lib/... for react-native . https://github.com/software-mansion/react-native-reanimated/blob/acf7429068c4f801eef41267c2a94b5515ec4ff8/package.json#L30

Same one for react-native-paper https://github.com/callstack/react-native-paper/blob/6ff9a20885ffcbe9c864542a26a6ee47db355002/package.json#L7

Also please address the failures in the CI

[naftalibeder]

@natew

Regarding the test failure of the pattern

'MapView' cannot be used as a JSX component.

I had the same thing in my now-cancelled Typescript conversion PR, and the only way I was able to fix it was adding these comments:

 *
 * @extends {Component<MapViewProps>}
 */
class MapView extends NativeBridgeComponent(Component) { ... }

Perhaps you can solve it another way, but I thought this might help.

[naftalibeder]

BTW if you look at react-native-reanimated they also point to src/index and not to lib/... for react-native . Same one for react-native-paper

@mfazekas it is worth noting that both of these projects use https://github.com/callstack/react-native-builder-bob, which does compile to lib and thereby has a very wide range of support.

I've also had a lot of trouble installing @rnmapbox/maps from a local copy (currently using npm pack for local dev ☹️), which by contrast has been easy to do with other projects built on react-native-builder-bob. So I'm not exactly advocating for rebuilding on top of that template, but compiling down to a 100% JS library does seem like a clean, effective pipeline.

[mfazekas]

BTW if you look at react-native-reanimated they also point to src/index and not to lib/... for react-native . Same one for react-native-paper

@mfazekas it is worth noting that both of these projects use https://github.com/callstack/react-native-builder-bob, which does compile to lib and thereby has a very wide range of support.

I've also had a lot of trouble installing @rnmapbox/maps from a local copy (currently using npm pack for local dev ☹️), which by contrast has been easy to do with other projects built on react-native-builder-bob. So I'm not exactly advocating for rebuilding on top of that template, but compiling down to a 100% JS library does seem like a clean, effective pipeline.

From react-native-builder-blob: https://github.com/callstack/react-native-builder-bob

It's usually good to point to your source code with the react-native field to make debugging easier. Metro already supports compiling a lot of new syntaxes including JSX, Flow and TypeScript and it will use this field if present.

So I don't think for react-native project that changes anything - unless you're not using metro, say for react-native-web. So sure I'm fine with doing transpiration esp. for non metro bundlers. But I doubt that it will make any difference for regular metro based rn projects.

[naftalibeder]

@mfazekas are you able to develop on this project locally, as in npm install /my/local/clone/maps in a parent RN project? If so, can you describe what your setup is?

I think this is one of the fundamental things that makes me support this PR, to make local development more predictable.

[natew]

Sorry for drive-by PR. Honestly, part of this is just that my tsc --watch on my app complains and I can't seem to find a way to disable it, despite trying a few different excludes, declares etc. But I think in general it's also just a nice practice, it means you can require() from a node REPL to introspect the shape of things, for example.

[mfazekas]

@mfazekas are you able to develop on this project locally, as in npm install /my/local/clone/maps in a parent RN project? If so, can you describe what your setup is?

I think this is one of the fundamental things that makes me support this PR, to make local development more predictable.

@naftalibeder but are they?!

I'm mostly using yarn, and I'm usually install from GitHub, (Btw. npm link would be awesome but symlinks don't work with metro used by RN). I'll try npm install as soon a I have some time.

I don't see what's the difference between npm install <path> and npm install rnmapbox/maps#main. You should be able to diff the node_modules/@rnmapbox/maps to see if they are any different, but I don't think so. What is the error you're seeing?!

[naftalibeder]

I'm mostly using yarn, and I'm usually install from GitHub, (Btw. npm link would be awesome but symlinks don't work with metro used by RN). I'll try npm install as soon a I have some time.

That would be great, or yarn add ../rnmapbox.

I don't see what's the difference between npm install <path> and npm install rnmapbox/maps#main. You should be able to diff the node_modules/@rnmapbox/maps to see if they are any different, but I don't think so. What is the error you're seeing?!

I don't really understand either, but the problem was that it was reading rnmapbox's node_modules and thereby reading in a conflicting duplicate version of React. I tried a bunch of metro magic to fix that but wasn't able to.

The weird thing is that when I tried doing npm install --save ../rnmapbox from a fresh RN project, it worked. So there's something weird about my project that I haven't figured out (weird because an install from github works fine).

[mfazekas]

I'm mostly using yarn, and I'm usually install from GitHub, (Btw. npm link would be awesome but symlinks don't work with metro used by RN). I'll try npm install as soon a I have some time.

That would be great, or yarn add ../rnmapbox.

I don't see what's the difference between npm install <path> and npm install rnmapbox/maps#main. You should be able to diff the node_modules/@rnmapbox/maps to see if they are any different, but I don't think so. What is the error you're seeing?!

I don't really understand either, but the problem was that it was reading rnmapbox's node_modules and thereby reading in a conflicting duplicate version of React. I tried a bunch of metro magic to fix that but wasn't able to.

Oh I see yes version checked out from git does not have node_modules while local version where you did yarn install does have node_modules . So that's a difference in install from GitHub and install from local dev version path.

[SkySails]

I have a multi-platform project where this library is used for maps on native platforms and the official JS SDK is used for web. The project uses react-native-web which has already been mentioned as a possible reason to start doing transpilation.

At the moment, there is no way to tell the typescript compiler to ignore this library when building the web version since the library is included (imported) in the project. The bundler (webpack) never actually includes any of its code because of platform-specific file extensions, but the library is still type-checked since tsc doesn't recognize said extensions.

I'm using several libraries that are built with RN in mind (which usually means that they include JSX in JS files), but this is the only library that causes issues - because it includes raw TS files. My current workaround can be found here, it would be awesome if this step was not necessary! 🙌

[mfazekas]

@SkySails so is this a tsc issue?! Would rn-tsc work for you until it’s resolved in lib?! https://microsoft.github.io/rnx-kit/docs/tools/typescript-react-native-compiler

[SkySails]

It absolutely could've, thanks for the tip! It's been a while since I last heard of rnx-kit, they have some really useful stuff!

Unfortunately, I'm using fork-ts-checker-webpack-plugin, and patching it in order to use rn-tsc instead would be about as hacky as the current workaround IMO. But for those running tsc themselves, that could be a potential (temporary) solution!

[mfazekas]

Folks, just to clarify i do accept a pr that transpiles to lib dir. Provided that:

• generated sources are not stored in git (for now)
• react-native keep pointing to ./javascript/index.js. Note that main or browser can point to lib/index.js or similar.

See

https://github.com/software-mansion/react-native-reanimated/blob/acf7429068c4f801eef41267c2a94b5515ec4ff8/package.json#L30

and

https://github.com/callstack/react-native-paper/blob/6ff9a20885ffcbe9c864542a26a6ee47db355002/package.json#L7

for reference

[YousefED]

react-native keep pointing to ./javascript/index.js. Note that main or browser can point to lib/index.js or similar.

@mfazekas if you insist on keeping this as-is (which some libraries that you mention seem to do indeed), I don't think there's any benefit to ship compiled JS in the first place right?

I would argue it's a bit of an anti-pattern, as it causes issues like this down the line: https://github.com/rnmapbox/maps/issues/2333, for example if there are different typescript settings.

However, I do understand that it has a benefit where you can keep referring to a github branch in package.json and everything "just works", like you explain here.

I think a cleaner way to get a github/branch-referenced package working (i.e.: create an app "DemoApp" that depends on rnmapbox#branch-name) is to:

a) point react-native in package.json to the transpiled, built files b) include a postinstall step in "DemoApp" that builds node_modules/rnmapbox.

Of course, the postinstall step would not be necessary when depending on a published version of rnmapbox.

What do you think?

[mfazekas]

react-native keep pointing to ./javascript/index.js. Note that main or browser can point to lib/index.js or similar.

@mfazekas if you insist on keeping this as-is (which some libraries that you mention seem to do indeed), I don't think there's any benefit to ship compiled JS in the first place right?

I don't think you're right about that. The bug report that contains the concrete issue Is #2333, which is about tsc with a custom config. Tsc doesn't use the react-native key from package.json so tsc related issues should be addressed.

[YousefED]

I don't think you're right about that. The bug report that contains the concrete issue Is https://github.com/rnmapbox/maps/issues/2333, which is about tsc with a custom config. Tsc doesn't use the react-native key from package.json so tsc related issues should be addressed.

Depends how you look at it :) The reason this is related is that this issue (#2333) wouldn't have occured if the typescript build process would have been decoupled from this package. By keeping it like this, you're implicitly forcing consumers to:

• use a specific Typescript version that's compatible with RNMapbox
• use specific settings for Typescript that are compatible with RNMapbox

This limits the re-usability of the library in my opinion

[mfazekas]

We should be now doing this with bob build