Adding Browser/WASM template for binding generator #5497

nhachicha · 2023-02-24T15:37:43Z

WIP adding browser template for binding gen.

Build the browser target.

cd packages/realm
./build-browser-target.sh

Run the sample react app

cd packages/web-example
npm install
npm start

observe the output in the Dev console

We're now relying 100% on the "bson" package.

includes workaround for microsoft/TypeScript#51261.

Add allSessions to SyncUser in bindgen. Transform result to JS SyncSession array.

…dgen

RedBeard0531

Posting initial feedback before reviewing the main browser.ts template file. I also skipped over the code in packages/realm since it looks like it isn't actually intended to be merged, and web-example since I know basically nothing about react.

@kraenhansen does it make sense to commit this change to package-lock.json or should we revert it for now. It seems like it is a full rewrite of that file. I never know how to handle that and usually just git checkout package-lock.json before committing.

packages/bindgen/CMakeLists.txt

packages/bindgen/package.json

packages/bindgen/src/templates/node-wrapper.ts

RedBeard0531 · 2023-03-01T10:31:20Z

packages/realm/rollup.config.mjs

+        resolveOnly: ["@realm/network-transport", "path-browserify"],
+      }),
+      // We need to use `commonjs` because of "path-browserify"
+      commonjs(),


It seems confusing that we need to use commonjs() when doing format: "es", which I think is the opposite of cjs. @kraenhansen can you look into this and suggest either removing it or wording for a comment that explains why this makes sense even when not output cjs?

RedBeard0531 · 2023-03-01T10:33:22Z

packages/realm/rollup.config.mjs

+      copy({
+        targets: [{ src: "./generated/ts/realm-js-wasm.wasm", dest: "./dist/" }],
+      }),


Can we instead just generate to the right place? I think this file is now being copied twice after generation which seems silly.

With the change introduced in #5497 (comment) the realm-js-wasm.was & realm-js-wasm.js will be generated from the bindgen without an extra copy step in cmake. These files will be under packages/realm/generated which is already used by other templates (node/react-native).
Rollup is already bundling realm-js-wasm.js but it's not aware of the wasm file, hence this manual copy to dist

src/browser/platform.cpp

packages/bindgen/CMakeLists.txt

fealebenpae · 2023-03-01T10:13:57Z

packages/bindgen/CMakeLists.txt

+            ${TS_FILES}
+    )
+
+    target_sources(realm-js PRIVATE browser_init.cpp ${CMAKE_JS_SRC} ../../src/browser/platform.cpp)


Suggested change

target_sources(realm-js PRIVATE browser_init.cpp ${CMAKE_JS_SRC} ../../src/browser/platform.cpp)

target_sources(realm-js PRIVATE browser_init.cpp ../../src/browser/platform.cpp)

CMAKE_JS_SRC is only relevant to Node.js.

packages/bindgen/CMakeLists.txt

packages/bindgen/src/templates/browser.ts

fealebenpae · 2023-03-01T10:40:06Z

packages/bindgen/src/templates/node-wrapper.ts

+    const nativeModule = await Module(); // loading WASM 
+    nativeModule.browserInit();


I think this is a good spot to inject things like UUID and Decimal128 on Module and recall them with val::module_property() in C++ instead of relying on the injectables mechanic.

I'd rather we stay consistent between the engines for now, unless there is a good reason to diverge. Is there some benefit to injecting them this way rather than using injectInjectables?

browserInit(); exists solely to compensate for the lack of "engine registration phase" (i.e NODE_API_NAMED_ADDON(realm_cpp, RealmAddon) or realm_jsi_init which is called from Java_io_realm_react_RealmReactModule_install) which will trigger the creation of the RealmAddon instance.

Registering the JS type should still be done via injectInjectables IMO to keep the separation of concerns ...

RedBeard0531 · 2023-03-01T10:47:20Z

@nhachicha Is it possible to revert your bump of the core submodule for now? I think we are planning on doing the bump soon in the bindgen branch anyway, so I'd rather not do it here too A) so that we don't hit an extra merge conflict and B) to keep this review focused on what is needed to add emscripten/browser support.

RedBeard0531 · 2023-03-01T12:10:52Z

packages/bindgen/src/templates/node-wrapper.ts

+        ...params,
+        asyncSig ? "_cb" : [],
+      ].flat();
+      let call = `${native}([${args}])`;


I didn't notice this change at first. Please change this back to just (${args}). We shouldn't be making a new JS Array every time we call a native function!

It's easier to access the args consistently this way in the generated C++ (we don't have a JS engine abstraction like in node and JSI that facilitate accessing the args). Also passing var args is not supported AFAIK from the Embind perspective.

You don't need to use varargs in C++. When generating the C++, we know exactly how many arguments are going to be passed, so we can just generate a function taking that many args. You can choose between naming the args arg1, arg2,.... so you just need to change arg[${i}] to arg${i}, or you can just use the arg names from the spec (a.name, with '_self' for the this parameter in non-static methods). Slight preference for using the meaningful names, but not a big deal either way.

You won't be able to check the number of passed args for error handling, but that is probably fine. I think we can get rid of that everywhere since it is probably unnecessary now that we have the generated JS code to ensure that we always pass the correct number of arguments (although some may be undefined)

Co-authored-by: Mathias Stearn <redbeard0531@gmail.com>

nhachicha · 2023-03-01T13:52:19Z

@RedBeard0531 regarding

Posting initial feedback before reviewing the main browser.ts template file. I also skipped over the code in packages/realm since it looks like it isn't actually intended to be merged, and web-example since I know basically nothing about react.

@kraenhansen does it make sense to commit this change to package-lock.json or should we revert it for now. It seems like it is a full rewrite of that file. I never know how to handle that and usually just git checkout package-lock.json before committing.

note that I npm ejected the sample app that @kraenhansen created to access the webpack configuration which allows us to work around an issue see https://github.com/realm/realm-js/pull/5497/files#diff-37e3ecdc2a16d68439cd2d7f3cbf0c0e14b2ac20fc650cb0f1764768e264c921R191-R193
(adding experiments: { topLevelAwait: true})
This has caused all the extra files you see here (and the update to the package-lock)

nhachicha · 2023-03-01T13:54:17Z

@RedBeard0531

@nhachicha Is it possible to revert your bump of the core submodule for now? I think we are planning on doing the bump soon in the bindgen branch anyway, so I'd rather not do it here too A) so that we don't hit an extra merge conflict and B) to keep this review focused on what is needed to add emscripten/browser support.

As discussed on slack, we should probably keep this PR as a feature branch until we have Emscripten support merged in Core/master.

Co-authored-by: Yavor Georgiev <fealebenpae@users.noreply.github.com>

nhachicha · 2023-03-03T11:09:00Z

superseded by #5525

kraenhansen and others added 30 commits December 28, 2022 13:00

Passing linking objects tests

b053b7d

Passing linking objects and object tests

8731e4e

Fixed dict constraining integration tests

ee85e4b

Fixing SDK tests

b4d0690

Fixing legacy tests

f973bf0

Deleting BSON unit tests

0fcc9b6

We're now relying 100% on the "bson" package.

Adopted the "internal module pattern"

2b82222

Fixed Results and Dictionary tests

fd5691a

Fixing encryotion, mixed, object and results tests

f7f47cd

Refixed propagating ordering when filtering

11f4e6c

Unbroke SDK tests & fix alias tests

2fa58da

Add Realm listeners (#5020)

e1ea790

add method to reset shared pointers eagerly

d6e128f

remove internal pointer from object prior to reset in $resetSharedPtr

b68784d

Reordering imports

c806c58

Ignoring only the generated types

4224200

Implementing migrations

ea9cd9d

Adding a primaryKeys chai property

b992639

Fixing migrations

a966e09

Fixing iterating tests

6f2a36b

Add $addr magic property

b8e064e

includes workaround for microsoft/TypeScript#51261.

Fixing app import test hook

f9f588e

Fixed reading response body

ff5f507

Adding sourcemap to the SDK bundle

8c07976

Adding App, Credentials and User scaffolding.

31377a0

Adding a bit more scaffoling

b5b6e17

Adding User and Session caching

8381f58

Add an initial .ignore file so rg and fd will look at generated sources

69840be

update core to v12.11.0

88a6bea

Adding base64 decoding to binary properies

bb32158

JacobOscarGunnarsson and others added 3 commits February 24, 2023 10:13

Ensure that Realm enums are accessible (#5484)

f14917b

Merge pull request #5487 from realm/fp/linking-objects

6b70ff0

Implement getAllSyncSessions (#5492)

5847a92

Add allSessions to SyncUser in bindgen. Transform result to JS SyncSession array.

nhachicha requested a review from kraenhansen February 24, 2023 15:37

cla-bot bot added the cla: yes label Feb 24, 2023

nhachicha requested a review from RedBeard0531 February 24, 2023 15:37

kraenhansen and others added 8 commits February 24, 2023 22:24

Adding a @realm/web-example package

52fe959

Adding a mocked browser target to SDK

83c1ac5

Using realm from the example app

d354ec6

Slimmed down the app

32dffcb

Adding Browser/WASM template for binding generator

b4cb9d4

Merge remote-tracking branch 'origin/kh/bindgen/web' into nh/wasm/bin…

69fcb3c

…dgen

using generated browser bundle file with rollup

86e4246

Added a React sample using the Realm API via the browser bindgen

e20b811

nhachicha force-pushed the nh/wasm/bindgen branch from 1b66ec8 to e20b811 Compare March 1, 2023 01:28

nhachicha requested a review from fealebenpae March 1, 2023 01:38

RedBeard0531 reviewed Mar 1, 2023

View reviewed changes

fealebenpae reviewed Mar 1, 2023

View reviewed changes

kraenhansen force-pushed the bindgen branch from 899cac3 to 7e2beeb Compare March 1, 2023 11:05

RedBeard0531 reviewed Mar 1, 2023

View reviewed changes

nhachicha and others added 2 commits March 1, 2023 13:08

Update packages/bindgen/package.json

0de4f51

Co-authored-by: Mathias Stearn <redbeard0531@gmail.com>

Update packages/bindgen/CMakeLists.txt

8fd2e85

Co-authored-by: Mathias Stearn <redbeard0531@gmail.com>

Update packages/bindgen/CMakeLists.txt

39249c3

Co-authored-by: Yavor Georgiev <fealebenpae@users.noreply.github.com>

nhachicha mentioned this pull request Mar 2, 2023

[bindgen] Adding Browser/WASM target to bindgen #5525

Draft

nhachicha closed this Mar 3, 2023

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Browser/WASM template for binding generator #5497

Adding Browser/WASM template for binding generator #5497

nhachicha commented Feb 24, 2023 •

edited

Loading

RedBeard0531 left a comment

RedBeard0531 Mar 1, 2023

RedBeard0531 Mar 1, 2023

nhachicha Mar 1, 2023

fealebenpae Mar 1, 2023

fealebenpae Mar 1, 2023

RedBeard0531 Mar 1, 2023

nhachicha Mar 1, 2023

RedBeard0531 commented Mar 1, 2023

RedBeard0531 Mar 1, 2023

nhachicha Mar 1, 2023

RedBeard0531 Mar 1, 2023

RedBeard0531 Mar 1, 2023

nhachicha commented Mar 1, 2023

nhachicha commented Mar 1, 2023

nhachicha commented Mar 3, 2023

	target_sources(realm-js PRIVATE browser_init.cpp ${CMAKE_JS_SRC} ../../src/browser/platform.cpp)
	target_sources(realm-js PRIVATE browser_init.cpp ../../src/browser/platform.cpp)

		const nativeModule = await Module(); // loading WASM
		nativeModule.browserInit();

Adding Browser/WASM template for binding generator #5497

Adding Browser/WASM template for binding generator #5497

Conversation

nhachicha commented Feb 24, 2023 • edited Loading

RedBeard0531 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedBeard0531 commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhachicha commented Mar 1, 2023

nhachicha commented Mar 1, 2023

nhachicha commented Mar 3, 2023

nhachicha commented Feb 24, 2023 •

edited

Loading