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

[bindgen] SDK packaging #5466

Merged
merged 6 commits into from
Feb 22, 2023
Merged

[bindgen] SDK packaging #5466

merged 6 commits into from
Feb 22, 2023

Conversation

kraenhansen
Copy link
Member

@kraenhansen kraenhansen commented Feb 21, 2023

What, How & Why?

This moves the Android source files needed to bootstrap the library into the SDK package to make them available for the dependent apps.
It also updates the package.json with conditional exports as speced here: https://nodejs.org/api/packages.html#conditional-exports

Unfortunately we can't make this a module: "type" wet, because of a bug in the React Native CLI which breaks reading the react-native.cli.js file (I still have to report this).

@kraenhansen kraenhansen self-assigned this Feb 21, 2023
@cla-bot cla-bot bot added the cla: yes label Feb 21, 2023
@kraenhansen kraenhansen changed the base branch from master to bindgen February 21, 2023 07:49
Copy link
Contributor

@elle-j elle-j left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +26 to +31
".": {
"types": "./dist/bundle.d.ts",
"node": "./dist/bundle.node.mjs",
"require": "./dist/bundle.node.js",
"react-native": "./dist/bundle.react-native.mjs"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat 😎

Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

The Android build still lands in the root react-native directory. That should be addressed.

@@ -20,6 +20,7 @@ const commandLineArgs = require("command-line-args");
const fs = require("fs-extra");
const path = require("path");
const exec = require("child_process").execFileSync;
const packageRoot = path.resolve(__dirname, "..");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move the build scripts into packages/realm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this will put the build artifacts in react-native instead of packages/realm/react-native. I think we want to actually delete the root project react-native folder and have everything within packages/realm/react-native, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

move the build scripts into packages/realm?

I was thinking about that. I see pros and cons to it. It probably makes sense to increase cohesion as these will only be used to build the SDK native parts. But then again, the CmakeLists.txt live in the bindgen package right now, which sort of serves the same purpose.

One my thought I had was that I want to refactor and join the build scripts into a single CLI that can invoke cmake when building for Node.js, iOS or Android with one (or more) platform and arch flags.

@takameyer
Copy link
Contributor

One my thought I had was that I want to refactor and join the build scripts into a single CLI that can invoke cmake when building for Node.js, iOS or Android with one (or more) platform and arch flags.

Also a good idea. It just seems to me that these scripts are only called from within packages/realm, so they could be transfered up. In any case, if we wrapped it into a single cli, it would still end up being a dependency of packages/realm.

@kraenhansen
Copy link
Member Author

The Android build still lands in the root react-native directory. That should be addressed.

@takameyer It was less baked than I remembered 🙂 But it's fixed now. Thanks!

Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

LGTM!

@kraenhansen kraenhansen merged commit e0db136 into bindgen Feb 22, 2023
@kraenhansen kraenhansen deleted the kh/bindgen/packaging branch February 22, 2023 09:22
papafe added a commit that referenced this pull request Feb 24, 2023
* bindgen:
  Implement getAllSyncSessions (#5492)
  Ensure that Realm enums are accessible (#5484)
  Apply suggestions from code review [skip ci]
  Small corrections [skip ci]
  Added changelog and final corrections
  Stub
  add test to validate that foreach throws on a dictionary (#5467)
  Using `RealmInsertionModel` on `Results#update`
  Updated "react-native" dev dep to 0.71.0
  Bumped lower bound on our RN peer dependency
  [bindgen] SDK packaging (#5466)
  Adding "prebuild" and configuring it (#5447)
  add synthetic private brand fields to TS wrappers for C++ classes, and fix found bug
  import bindings directly rather than through internal
  Stub work
kraenhansen added a commit that referenced this pull request Mar 1, 2023
* Moved Android SDK files

* Enumerate files to include in package

* Adding platform build npm scripts

* Updated "exports" object

* Removed check for "react-native" existence.

* Updated `LIBRARY_OUTPUT_DIRECTORY`
kraenhansen added a commit that referenced this pull request Mar 3, 2023
* Moved Android SDK files

* Enumerate files to include in package

* Adding platform build npm scripts

* Updated "exports" object

* Removed check for "react-native" existence.

* Updated `LIBRARY_OUTPUT_DIRECTORY`
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants