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

Documentation III #125

Merged
merged 10 commits into from
Oct 16, 2024
Merged

Documentation III #125

merged 10 commits into from
Oct 16, 2024

Conversation

jhugman
Copy link
Owner

@jhugman jhugman commented Oct 15, 2024

Fixes #121

This PR completes the baseline documentation (I think).

It adds:

  • internals - sketching:
    • the codegen'd path that the generate turbo-modules uses
    • the lifting and lowering path that the generated bindings uses
  • callback interfaces and async-callbacks
  • contribution guide for documentation.
  • miscellaneous typo fixes and refactoring

Reviewing theses docs has a guide now!

@Johennes : this page could do with your review/input.

@jhugman jhugman requested a review from zzorba October 15, 2024 14:37

## Who is using `uniffi-bindgen-react-native`?

- [@unomed/react-native-matrix-sdk](https://www.npmjs.com/package/@unomed/react-native-matrix-sdk)
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

docs/src/contributing/documentation.md Outdated Show resolved Hide resolved
docs/src/contributing/documentation.md Outdated Show resolved Hide resolved
docs/src/contributing/documentation.md Outdated Show resolved Hide resolved

## Binary builds

In order to distribute pre-built packages you will need to add to `.gitignore` the built `.a`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would reword this slightly. The main reason you'll want to add rust_modules/ and *.a to .gitignore is so that you don't track the large binaries in git (and the getting started page already gives that tip). The issue though is that when publishing, npm will also use .gitignore to decide which files to include in the package.

This can be worked around in two ways:

  1. By using .npmignore. This will require you to duplicate those ignore patterns that git and npm have in common (which will be most of them). I personally think this isn't a good option because it's quite a footgun. You'll always have to remember to duplicate new entries as you add them in future.
  2. By applying some clever logic by overriding parts of .gitignore with the files array in package.json.

docs/src/guides/publishing.md Outdated Show resolved Hide resolved

## Source packages

If asking your users to compile Rust source is acceptable, then adding a `postinstall` script to `package.json` may be enough.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might also be helpful to advise remind consumers to build the Rust crate in release mode when shipping their projects? Maybe that's obvious. I just thought of it because the size difference can be enormous.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Another excellent suggestion.


but add them back in to the `files` section of `package.json`[^issue121].

[^issue121]: [This advice](https://github.com/jhugman/uniffi-bindgen-react-native/issues/121) is from @Johennes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if it should go here or into #121 but it's probably a good idea run npm pack --dry-run and inspect the package contents before doing any actual publishing. I guess that's probably good practice for any npm release but it might be worth pointing out explicitly here due to the ignore trickery.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think you're right: we should land this and the improve this page with #121. Rather than me re-wording (without knowledge) your comment, would you mind raising a PR?

docs/src/idioms/async-callbacks.md Outdated Show resolved Hide resolved
@@ -136,7 +135,6 @@ Until then, you need to add the dependency to the app's Podfile, in this case `e
+ # We need to specify this here in the app because we can't add a local dependency within
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is publishing to cocoapods still an option?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Publishing uniffi-bindgen-react-native? yes definitely.

}
log(message: string) {
if (!this.isEnabled()) {
throw new MyError.LoggingDisabled();
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if I throw an error that doesn't conform to the MyError declared in the rust code?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Aha. I'd missed this. Thanks for spotting that.

I've added some more docs.

Copy link
Collaborator

@zzorba zzorba left a comment

Choose a reason for hiding this comment

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

LGTM, one small question/clarification

jhugman and others added 2 commits October 16, 2024 11:34
@jhugman jhugman merged commit d25da35 into main Oct 16, 2024
1 check passed
@jhugman jhugman deleted the jhugman/docs-iii branch October 16, 2024 12:03
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.

Document steps for how to publish a uniffi'd project on npmjs
3 participants