maps
maps copied to clipboard
split out compiled js from typescript types and autogenerate both
Fixes https://github.com/rnmapbox/maps/issues/2140#issuecomment-1230278406
@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:
- Shouldn't the
prepare
script include compiling/packaging? It gets run onnpm install
, so without that, it'll be hard to install the library directly from Github, or from a local copy. - 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.
@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 atsc watch
is possible but not ideal.) - [ ] use package with
yarn add rnmapbox/maps#main
andnpm 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
@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.
BTW if you look at
react-native-reanimated
they also point tosrc/index
and not tolib/...
forreact-native
. Same one forreact-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.
BTW if you look at
react-native-reanimated
they also point tosrc/index
and not tolib/...
forreact-native
. Same one forreact-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 usingnpm pack
for local dev ☹️), which by contrast has been easy to do with other projects built onreact-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.
@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.
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 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?!
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>
andnpm install rnmapbox/maps#main
. You should be able to diff thenode_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).
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>
andnpm install rnmapbox/maps#main
. You should be able to diff thenode_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.
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 (import
ed) 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! 🙌
@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
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!
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 thatmain
orbrowser
can point tolib/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
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?
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.
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
We should be now doing this with bob build