Binding resolution refactored #5802

kraenhansen · 2023-05-09T12:29:53Z

What, How & Why?

This fixes part of #5576 by declaring the output of the binding generator as a virtual "realm/binding" module, which we provide a path for implementation via tsconfig paths: A default fallback for tests (in packages/realm/tsconfig.json) as well as each of the platform specific directories.

It also removes the typescript-checker, as we concluded this might just be deferred and will happen as as part of building the SDK anyway.

kraenhansen · 2023-05-09T12:30:38Z

packages/realm/bindgen/src/templates/typescript.ts

-    coreOut("};");
-  }
-  coreOut(`
+  out("declare module 'realm/binding' {");


This is one of the more important changes allowing us to import from "realm/binding" via paths in the tsconfig and have the types resolve as expected.

I think you are missing a word after "without"

Thanks - I updated the comment 👍

I still don't love that this file is picked up by the src/*.ts glob and injects its contents into a virtual module rather than being found via an import path. But I don't currently have a great idea for an alternative if we want to move away from rollup's replace plugin.

packages/realm/bindgen/src/templates/typescript.ts

kraenhansen · 2023-05-09T12:35:46Z

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

+  both("// Enums");
+  for (const e of spec.enums) {
+    // This pattern is copied from the TS compiler
+    both(`export var ${e.jsName};`);
+    both(`(function (${e.jsName}) {`);
+    both(
+      ...e.enumerators.map(({ jsName, value }) => `${e.jsName}[${e.jsName}["${jsName}"] = ${value}] = "${jsName}";`),
+    );
+    both(`})(${e.jsName} || (${e.jsName} = {}));`);
+  }


Probably the most controversial change, but not a large price to pay to avoid relying on code replacements from the Rollup config.

const enums don't generate any js code: https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBAYwgOwM72MgrgWzgMQgjgG8BYAKDmrgDMiAaOAIwEMom2AvSgXyA. It only adds declarations to the .d.ts file.

I believe const enum is only supposed to be used in non-declaration files.

Here's an example:

{ "name": "ts-const-enums", "private": true, "type": "module", "scripts": { "test": "tsx index.ts" }, "dependencies": { "tsx": "^3.12.7", "typescript": "^5.0.4" } }

// index.ts import { Foo } from "./enums"; console.log(Foo.bar);

// enums.d.ts export const enum Foo { bar = "bar" }

// enums.js // note how we provide no implementation

Running npm test yeilds:

/Users/kraen.hansen/Projects/ts-const-enums/index.ts:1 import { Foo } from "./enums"; ^ SyntaxError: The requested module './enums' does not provide an export named 'Foo' at ModuleJob._instantiate (node:internal/modules/esm/module_job:123:21) at async ModuleJob.run (node:internal/modules/esm/module_job:189:5) at async Promise.all (index 0) at async ESMLoader.import (node:internal/modules/esm/loader:527:24) at async loadESM (node:internal/process/esm_loader:91:5) at async handleMainPromise (node:internal/modules/run_main:65:12)

Even if we do provide a dummy implementation of Foo in enums.js:

export const Foo = {};

This simply prints undefined.

Maybe it behaves differently for string vs number const enums? When I tested locally with this patch, everything seems to work:

diff --git a/packages/realm/bindgen/src/templates/node-wrapper.ts b/packages/realm/bindgen/src/templates/node-wrapper.ts index c66cf638c..364d7d840 100644 --- a/packages/realm/bindgen/src/templates/node-wrapper.ts +++ b/packages/realm/bindgen/src/templates/node-wrapper.ts @@ -131,17 +131,6 @@ export function generate({ spec: rawSpec, file }: TemplateContext): void { } `); - both("// Enums"); - for (const e of spec.enums) { - // This pattern is copied from the TS compiler - both(`export var ${e.jsName};`); - both(`(function (${e.jsName}) {`); - both( - ...e.enumerators.map(({ jsName, value }) => `${e.jsName}[${e.jsName}["${jsName}"] = ${value}] = "${jsName}";`), - ); - both(`})(${e.jsName} || (${e.jsName} = {}));`); - } - const injectables = [ "Long", "ArrayBuffer", diff --git a/packages/realm/bindgen/src/templates/typescript.ts b/packages/realm/bindgen/src/templates/typescript.ts index 3210f1f5c..0091d5f34 100644 --- a/packages/realm/bindgen/src/templates/typescript.ts +++ b/packages/realm/bindgen/src/templates/typescript.ts @@ -203,7 +203,7 @@ export function generate({ spec: rawSpec, file }: TemplateContext): void { out("// Enums"); for (const e of spec.enums) { // Using const enum to avoid having to emit JS backing these - out(`export enum ${e.jsName} {`); + out(`export const enum ${e.jsName} {`); out(...e.enumerators.map(({ jsName, value }) => `${jsName} = ${value},\n`)); out("};"); }

Of course, I've often wondered whether we should be using enum or const enum for these. const enum may provide some perf and size benefits, but I don't know how much that matters given the JIT in node and the bytecode compiler for hermes. But it certainly shouldn't be slower, and I don't think we've needed the features of a non-const enum yet (eg mapping numeric values to/from string names at runtime)

When I apply this, the SDK unit test fails with:

✖ ERROR: TypeError: Cannot read properties of undefined (reading 'Get') at <anonymous> (/Users/kraen.hansen/Projects/realm-js/packages/realm/src/platform/network.ts:35:23) at Object.<anonymous> (/Users/kraen.hansen/Projects/realm-js/packages/realm/src/platform/network.ts:62:17)

RedBeard0531 · 2023-05-09T14:17:37Z

Do you mind holding off on any changes to bindgen until I get #5769 over the line. I hope to have it merged today or tomorrow.

RedBeard0531 · 2023-05-10T09:11:46Z

packages/realm/bindgen/src/templates/typescript.ts

+    // Using const enum to avoid having to emit JS backing these
+    out(`export enum ${e.jsName} {`);


You lost the const in const enum

This is on purpose. Having a const enum in a non .d.ts file made the tests fail as it didn't have values at runtime. I suppose TSC only inlines these if they're in a TS source file.

RedBeard0531 · 2023-05-10T09:12:48Z

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

+  both("// Enums");
+  for (const e of spec.enums) {
+    // This pattern is copied from the TS compiler
+    both(`export var ${e.jsName};`);
+    both(`(function (${e.jsName}) {`);
+    both(
+      ...e.enumerators.map(({ jsName, value }) => `${e.jsName}[${e.jsName}["${jsName}"] = ${value}] = "${jsName}";`),
+    );
+    both(`})(${e.jsName} || (${e.jsName} = {}));`);
+  }


const enums don't generate any js code: https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBAYwgOwM72MgrgWzgMQgjgG8BYAKDmrgDMiAaOAIwEMom2AvSgXyA. It only adds declarations to the .d.ts file.

packages/realm/bindgen/src/templates/typescript.ts

RedBeard0531 · 2023-05-10T13:36:28Z

packages/realm/bindgen/CMakeLists.txt

Delete line 132 if you remove core.ts.

kraenhansen · 2023-05-11T09:41:01Z

I've pushed a commit pointing core to realm/realm-core#6604, which I'll change once the Core PR has been merged and released.

RedBeard0531

Comments are just suggestions. Everything looks correct.

RedBeard0531 · 2023-05-11T11:10:02Z

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

+  function both(...content: string[]) {
+    reactLines.push(...content);
+    nodeLines.push(...content);


This change now seems unnecessary. But also probably harmless. 🤷

RedBeard0531 · 2023-05-11T11:15:03Z

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

@@ -20,14 +20,15 @@ import { bindModel, Property } from "@realm/bindgen/bound-model";
 import { TemplateContext } from "@realm/bindgen/context";

 import { doJsPasses } from "../js-passes";
+import { eslintFormatter } from "../formatters/eslint-formatter";


I'm not sure it is worth introducing a new directory for just this one file. Personally, since the formatters are tiny, I would just put all of them in a single ../formaters.ts file, even if we had more than one.

But at the very least, lets remove some redundancy in src/formatters/eslint-formatter.ts. If you want to keep one file per formatter, I'd go either src/formatters/eslint.ts or just src/eslint-formatter.ts. Slight preference for the latter if you don't want just src/formatters.ts

Moved it one level up again 👍

RedBeard0531 · 2023-05-11T11:18:41Z

packages/realm/bindgen/src/templates/node.ts

@@ -31,6 +31,7 @@ import {
  Pointer,
  Template,
 } from "@realm/bindgen/bound-model";
+import { clangFormatter } from "@realm/bindgen/formatter";


It feels a bit weird to call this clangFormatter since the tool is named clang-format, and the tool called clang doesn't do any formatting. I admit that clangFormatFormatter sounds ridiculous though. Maybe we should just call the formatters by the name of the tool with no suffix, so just clangFormat and eslint?

RedBeard0531 · 2023-05-11T11:21:38Z

packages/realm/bindgen/src/templates/typescript.ts

@@ -162,7 +149,7 @@ export function generate({ spec: rawSpec, file }: TemplateContext): void {

  const spec = doJsPasses(bindModel(rawSpec));

-  const coreOut = file("core.ts", "eslint", "typescript-checker");
+  const coreOut = file("core.ts", eslintFormatter);


Suggested change

const coreOut = file("core.ts", eslintFormatter);

// This file is imported by the virtual path "realm/binding/core" in TS.

const coreOut = file("core.ts", eslintFormatter);

RedBeard0531 · 2023-05-11T11:25:48Z

packages/realm/bindgen/src/templates/typescript.ts

-    coreOut("};");
-  }
-  coreOut(`
+  out("declare module 'realm/binding' {");


I still don't love that this file is picked up by the src/*.ts glob and injects its contents into a virtual module rather than being found via an import path. But I don't currently have a great idea for an alternative if we want to move away from rollup's replace plugin.

RedBeard0531 · 2023-05-11T11:28:34Z

packages/realm/bindgen/src/templates/typescript.ts

@@ -216,7 +205,7 @@ export function generate({ spec: rawSpec, file }: TemplateContext): void {

  out("// Opaque types (including Key types)");
  for (const { jsName } of (spec.opaqueTypes as NamedType[]).concat(spec.keyTypes)) {
-    out.lines("/** Using an empty enum to express a nominal type */", `export declare enum ${jsName} {}`);
+    out.lines("/** Using an empty enum to express a nominal type */", `export enum ${jsName} {}`);


Suggested change

out.lines("/** Using an empty enum to express a nominal type */", `export enum ${jsName} {}`);

out.lines("/** Using an empty enum to express a nominal type */", `export const enum ${jsName} {}`);

We should probably consider some of the alternative mechanisms we discovered for opaque types at some point. But at the very least, we shouldn't imply that there is any actual value with that name.

How about just simply declaring these as abstract classes with a private brand?

https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEUBGBnALjKY30lFK8AYgPYnwDeAsAFDz3wAOMAlgG5RoJKYB2wAbloBfWrVB44iVBiw48BeACFYlWgyasOXeDyj8hNUTXHhoUsCV7pdsAFzLYhy9ZwAzMg9LkAvLZiGppIIbgCuvNgsVvBuvAAUHiReZACUDmwkLIK0sXFIsCmGQA

You mean like we already do for classes? Yes, possibly something like that. I wish there was a way to tell TS to model the behavior of Object.create(null) correctly, so that we could create a class that really had no known properties, but that doesn't seem possible right now.

RedBeard0531 · 2023-05-11T11:33:23Z

packages/realm/src/binding.ts


 /** @internal */
-export * from "../generated/ts/native.mjs"; // core types are transitively exported.
+export * from "realm/binding";


I know we just spend a long time figuring out how to make the re-export of every type from core.ts work through native.d.mts, but what if we just ... didn't. Do you think it would actually be cleaner to have this file do the export * from "../generated/ts/core"; rather than doing that in the wrapper and the .d.ts? That might let you remove the virtual realm/binding/core path.

Feel free to ignore if you are just tired of fucking around with that file 😁

RedBeard0531 · 2023-05-11T11:38:34Z

packages/realm/src/platform/node/fs.ts

-import { inject } from "../platform/file-system";
-import { extendDebug } from "../debug";
+import { inject } from "../file-system";
+import { extendDebug } from "../../debug";


We should consider defining a virtual path REALM/ or src/ so that imports look the same regardless of where you are in the directory structure. It is really unfortunate that when you move a file around, you need to modify its own imports, not just the imports of it. This is pretty common in C++ projects to always use "fully qualified" paths in #includes, so you use #include "realm/object-store/object.hpp" rather than #include "./object.hpp" if you are in realm/object-store/blah.hpp.

I think that's a good idea, but I'll leave it for a future PR 👍 Ideally I would also like to find a way to enforce that we import through the internal module instead of relative paths (for all other files than the platform specific).

RedBeard0531 · 2023-05-11T11:42:19Z

packages/realm/tsconfig.json

Is this file only used for editors and bundle.d.ts generation?

I believe so.

Pulled in latest bindgen branch from core

kraenhansen · 2023-05-15T11:12:23Z

I'll merge now, as the two failing "Test Coverage" scripts seem very unrelated to the change I'm proposing.

Follow-up to #5802 - we shouldn't use declare in a "declare module" block.

* main: Pushing update to the package-lock.json Binding resolution refactored (#5802) Update install-test-react-native.yml (#5808) Add more testing of IN operator (#5809)

kraenhansen added the T-Internal label May 9, 2023

kraenhansen self-assigned this May 9, 2023

cla-bot bot added the cla: yes label May 9, 2023

kraenhansen requested a review from RedBeard0531 May 9, 2023 12:29

kraenhansen commented May 9, 2023

View reviewed changes

packages/realm/bindgen/src/templates/typescript.ts Show resolved Hide resolved

kraenhansen commented May 9, 2023

View reviewed changes

kraenhansen added 2 commits May 10, 2023 10:56

Moved "node" and "react-native" into "src/platform"

ecfe106

Renaming platform specific outputs from bindgen

14d9e9a

kraenhansen force-pushed the kh/binding-resolution branch from 5f08a18 to a7ee197 Compare May 10, 2023 08:56

RedBeard0531 reviewed May 10, 2023

View reviewed changes

packages/realm/bindgen/CMakeLists.txt Outdated

Copy link

Contributor

RedBeard0531 May 10, 2023

Choose a reason for hiding this comment

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

Delete line 132 if you remove core.ts.

Using ts paths to resolve realm/binding

3ffb6d6

kraenhansen force-pushed the kh/binding-resolution branch from a7ee197 to 3ffb6d6 Compare May 11, 2023 08:57

kraenhansen requested a review from RedBeard0531 May 11, 2023 09:25

Passing formatters as functions

a336b2c

kraenhansen force-pushed the kh/binding-resolution branch from bd6a8cf to c42166a Compare May 11, 2023 10:21

RedBeard0531 approved these changes May 11, 2023

View reviewed changes

kraenhansen added 3 commits May 11, 2023 15:56

Ditch the typescript checker

165f85e

Simplified formatter naming

7385cba

Moved eslint formatter

1a81cb7

Pulled in latest bindgen branch from core

kraenhansen force-pushed the kh/binding-resolution branch from af8ec30 to 0dda196 Compare May 11, 2023 14:01

Temporarily update core

49f3d4e

kraenhansen force-pushed the kh/binding-resolution branch from 0dda196 to 49f3d4e Compare May 11, 2023 17:38

Upgrading core to pull in the lint

d5aafd5

kraenhansen merged commit 6993caf into main May 15, 2023

kraenhansen deleted the kh/binding-resolution branch May 15, 2023 11:12

kraenhansen added a commit that referenced this pull request May 16, 2023

Update typescript.ts

fb7de6e

Follow-up to #5802 - we shouldn't use declare in a "declare module" block.

papafe added a commit that referenced this pull request May 17, 2023

Merge branch 'main' into fp/minimal-fts

b2fad35

* main: Pushing update to the package-lock.json Binding resolution refactored (#5802) Update install-test-react-native.yml (#5808) Add more testing of IN operator (#5809)

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

Binding resolution refactored #5802

Binding resolution refactored #5802

kraenhansen commented May 9, 2023 •

edited

Loading

kraenhansen May 9, 2023 •

edited

Loading

RedBeard0531 May 10, 2023

kraenhansen May 11, 2023

RedBeard0531 May 11, 2023

kraenhansen May 9, 2023

RedBeard0531 May 10, 2023

kraenhansen May 10, 2023

RedBeard0531 May 10, 2023

kraenhansen May 10, 2023

RedBeard0531 commented May 9, 2023

RedBeard0531 May 10, 2023

kraenhansen May 10, 2023

RedBeard0531 May 10, 2023

RedBeard0531 May 10, 2023

kraenhansen commented May 11, 2023 •

edited

Loading

RedBeard0531 left a comment

RedBeard0531 May 11, 2023

RedBeard0531 May 11, 2023

kraenhansen May 11, 2023

RedBeard0531 May 11, 2023

RedBeard0531 May 11, 2023

RedBeard0531 May 11, 2023

RedBeard0531 May 11, 2023

kraenhansen May 11, 2023 •

edited

Loading

RedBeard0531 May 11, 2023

RedBeard0531 May 11, 2023

RedBeard0531 May 11, 2023

kraenhansen May 11, 2023

RedBeard0531 May 11, 2023

kraenhansen May 11, 2023

kraenhansen commented May 15, 2023

		// Using const enum to avoid having to emit JS backing these
		out(`export enum ${e.jsName} {`);

	const coreOut = file("core.ts", eslintFormatter);
	// This file is imported by the virtual path "realm/binding/core" in TS.
	const coreOut = file("core.ts", eslintFormatter);

	out.lines("/** Using an empty enum to express a nominal type */", `export enum ${jsName} {}`);
	out.lines("/** Using an empty enum to express a nominal type */", `export const enum ${jsName} {}`);

Binding resolution refactored #5802

Binding resolution refactored #5802

Conversation

kraenhansen commented May 9, 2023 • edited Loading

What, How & Why?

kraenhansen May 9, 2023 • edited Loading

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

Choose a reason for hiding this comment

RedBeard0531 commented May 9, 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

kraenhansen commented May 11, 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

kraenhansen May 11, 2023 • edited Loading

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

kraenhansen commented May 15, 2023

kraenhansen commented May 9, 2023 •

edited

Loading

kraenhansen May 9, 2023 •

edited

Loading

kraenhansen commented May 11, 2023 •

edited

Loading

kraenhansen May 11, 2023 •

edited

Loading