Replace "rollup" bundling with TypeScript project references #6492

kraenhansen · 2024-02-16T13:10:01Z

What, How & Why?

This is another stab at fixing #5894:

Deleted a usage of Rollup and related files in the SDK package.
Renamed the bundle NPM script in the SDK to build:ts, since it's no longer bundling.
Introduced separate tsconfig files for each runtime environment (Node.js, React-Native) + for tests and generating public types.
Introduced injection of the binding in platform/*/binding.ts. This was handled by Rollup before, but its not possible for the shared code to depend on the platform specific bindings, just based on file-locations. I experimented with using a self-referencing subpath export, but learned this was not fully supported by Metro. I believe this approach of injecting the binding can eventually help us to avoid exporting a top-level await on our WASM endpoint.
Removed all the named exports from ./index.ts since they we're not actually accessible externally anyway (we're using exports = Realm). I hope to be able to introduce these in a follow-up PR adding experimental ESM support for the package 🤞
The binding generator generates native.node.mjs and native.react-native.mjs, which are both ESM modules, but we need to be able to load them from the CJS SDK, so I've added a bindgen:transpile NPM script (dependent on by build:ts), which invoke babel to transform ESM to CJS in these files.
Rollup was generally happy to consume ESM or CJS, it didn't really care about file extensions etc, as all internal module references were removed during bundling anyway. I've spent a lot of time diving into TS modules, their use and limitations. Consider reading their theory for a more in-depth understanding.

If merged, I believe we can close #5853 as well.

Considerations

No longer including `@realm/fetch`

Since we're no longer bundling, there's no easy way to include the @realm/fetch code with the package.
We can probably just start publishing it to NPM, but it does add complexity and the risk of us forgetting to publish it or update the version in the SDK as it changes. If it worked, we might have included it as a bundled dependency. Perhaps we could do some symlink magic or find some other way to get it into the SDK package. Alternatively give in and use rollup, just for that. I haven't settled on this internally.

We're using `stripInternal`

We're relying on the stripInternal ts-config.
This was already the case with Rollup, so it's not a new problem introduced by this PR, but the rollup-plugin-dts plugin would perform a type-check, which we can no longer rely on. This makes us vulnerable to a situation where we accidentally forget an @internal annotation which relies on a type that is stripped from the package.

The use of the option has the following caveat in the docs:

This is an internal compiler option; use at your own risk, because the compiler does not check that the result is valid. If you are searching for a tool to handle additional levels of visibility within your d.ts files, look at api-extractor.

I spent quite some time trying to configure api-extractor, as recommended, but the use of self-referring subpath exports (which I was relying on at the time) is very confusing to the tool. It might be better now that I've reverted that and it's probably worth re-visiting.

For now, I've added a type-check NPM script, which builds the public types and tries building a project including them. This will fail if types are referenced which have been stripped from the public .d.ts files.

☑️ ToDos

📝 Changelog entry
📝 Compatibility label is updated or copied from previous entry
📝 Update COMPATIBILITY.md
🚦 Tests
📦 Updated internal package version in consuming package.jsons (if updating internal packages)
📱 Check the React Native/other sample apps work if necessary
💥 Breaking label has been applied or is not necessary

… a base

elle-j

I see a lot of work went into preparing and doing these great cleanups and refactoring! LGTM as soon as the tests are 🟢 👍 (plus comments)

elle-j · 2024-02-22T08:51:53Z

packages/realm-react/src/UserProvider.tsx

@@ -17,6 +17,7 @@
 ////////////////////////////////////////////////////////////////////////////

 import React, { createContext, useContext, useEffect, useState } from "react";
+import Realm from "realm";


You may be able to only import the type here.

Suggested change

import Realm from "realm";

import type Realm from "realm";

elle-j · 2024-02-22T09:15:50Z

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

+// eslint-disable-next-line @typescript-eslint/no-unused-vars
+export function generate({ spec: boundSpec, file }: TemplateContext, out: Outputter): void {


Suggested change

// eslint-disable-next-line @typescript-eslint/no-unused-vars

export function generate({ spec: boundSpec, file }: TemplateContext, out: Outputter): void {

export function generate({ spec: boundSpec }: TemplateContext, out: Outputter): void {

elle-j · 2024-02-22T09:18:21Z

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

 import { eslint } from "../eslint-formatter";
+import { generate as generateBase, generateNativeBigIntSupport } from "./base-wrapper";

 // eslint-disable-next-line @typescript-eslint/no-unused-vars


Suggested change

// eslint-disable-next-line @typescript-eslint/no-unused-vars

elle-j · 2024-02-22T09:21:43Z

packages/realm/bindgen/src/templates/react-native-wrapper.ts

+import { eslint } from "../eslint-formatter";
+import { generate as generateBase, generateNativeBigIntSupport } from "./base-wrapper";
+
+// eslint-disable-next-line @typescript-eslint/no-unused-vars


Suggested change

// eslint-disable-next-line @typescript-eslint/no-unused-vars

elle-j · 2024-02-22T09:35:52Z

packages/realm/package.json

@@ -86,6 +82,8 @@
    "build:ios:debug:simulator": "wireit",
    "build:ios:debug:ios": "wireit",
    "build:ios:debug:catalyst": "wireit",
+    "check-types": "wireit",
+    "clean": "git clean -fdx -e node_modules",


Is this needed here as well? (In addition to the top-level file.)

I don't remember why I choose to add it here as well, but I'll leave it out 👍

elle-j · 2024-02-22T10:28:09Z

packages/realm/src/app-services/User.ts

-  mongoClient(serviceName: string): MongoDB {
+  mongoClient(serviceName: string): MongoDBService {


Has the MongoDBService type been publicly exported with the same name as well?

elle-j · 2024-02-22T10:33:13Z

packages/realm/src/deprecated-global.ts

+    /** @deprecated Will be removed in v13.0.0. Please use {@link internal.AppServicesFunction} */
+    export import RealmFunction = internal.AppServicesFunction;
+    /** @deprecated Will be removed in v13.0.0. Please use {@link internal.CanonicalPropertySchema} */
+    export import CanonicalObjectSchemaProperty = internal.CanonicalPropertySchema;
+    /** @deprecated Will be removed in v13.0.0. Please use {@link internal.ClientResetRecoverUnsyncedChangesConfiguration} */
+    export import ClientResetRecoveryConfiguration = internal.ClientResetRecoverUnsyncedChangesConfiguration;
+    /** @deprecated Will be removed in v13.0.0. Please use {@link internal.PropertySchema} */
+    export import ObjectSchemaProperty = internal.PropertySchema;
+    /** @deprecated Will be removed in v13.0.0. Please use {@link internal.RealmObjectConstructor} */


Suggested change

/** @deprecated Will be removed in v13.0.0. Please use {@link internal.AppServicesFunction} */

export import RealmFunction = internal.AppServicesFunction;

/** @deprecated Will be removed in v13.0.0. Please use {@link internal.CanonicalPropertySchema} */

export import CanonicalObjectSchemaProperty = internal.CanonicalPropertySchema;

/** @deprecated Will be removed in v13.0.0. Please use {@link internal.ClientResetRecoverUnsyncedChangesConfiguration} */

export import ClientResetRecoveryConfiguration = internal.ClientResetRecoverUnsyncedChangesConfiguration;

/** @deprecated Will be removed in v13.0.0. Please use {@link internal.PropertySchema} */

export import ObjectSchemaProperty = internal.PropertySchema;

/** @deprecated Will be removed in v13.0.0. Please use {@link internal.RealmObjectConstructor} */

/** @deprecated Will be removed in v13.0.0. Please use {@link internal.AppServicesFunction | AppServicesFunction} */

export import RealmFunction = internal.AppServicesFunction;

/** @deprecated Will be removed in v13.0.0. Please use {@link internal.CanonicalPropertySchema | CanonicalPropertySchema} */

export import CanonicalObjectSchemaProperty = internal.CanonicalPropertySchema;

/** @deprecated Will be removed in v13.0.0. Please use {@link internal.ClientResetRecoverUnsyncedChangesConfiguration | ClientResetRecoverUnsyncedChangesConfiguration} */

export import ClientResetRecoveryConfiguration = internal.ClientResetRecoverUnsyncedChangesConfiguration;

/** @deprecated Will be removed in v13.0.0. Please use {@link internal.PropertySchema | PropertySchema} */

export import ObjectSchemaProperty = internal.PropertySchema;

/** @deprecated Will be removed in v13.0.0. Please use {@link internal.RealmObjectConstructor | RealmObjectConstructor} */

elle-j · 2024-02-22T10:34:59Z

packages/realm/src/platform/binding-patch.ts

+
+/**
+ * Applies SDK level patches to the binding.
+ * NOTE: This should only be called after the binding has been injected.


Suggested change

* NOTE: This should only be called after the binding has been injected.

* @note This should only be called after the binding has been injected.

I can't find any documentation on this jsdoc / tsdoc tag 🤔

I believe it primarily makes it more visible (highlighted) that it's a note.

elle-j · 2024-02-22T10:40:55Z

packages/realm/src/platform/react-native/fs.ts

+import JsPlatformHelpers = binding.JsPlatformHelpers;
+import Helpers = binding.Helpers;


I've read a bit about the special TS syntax, but for this case, I guess it didn't work to use const here?

Correct - const only handles the value, whereas I need this to copy the namespace type as well.

elle-j · 2024-02-22T11:10:43Z

packages/realm/tsconfig.react-native.json

+    "rootDir": "src/platform/react-native",
+    "sourceMap": true,
+    "resolveJsonModule": true,
+    "noEmit": false,


This is the default value right? (P.S. the node ts config file did not explicitly specify it)

The react-native config that we extend sets this to true, so we need to override that here.

Ah okay! 👍

kneth

I will have to read https://www.typescriptlang.org/docs/handbook/modules/theory.html carefully before my next round

kneth · 2024-02-22T08:30:05Z

packages/fetch/tsconfig.node.cjs.json

@@ -2,7 +2,8 @@
  "extends": "./tsconfig.node.base.json",
  "compilerOptions": {
    "outDir": "./dist/node-cjs",
-    "module": "CommonJS"
+    "module": "Node16",


According to the docs, it should be node16.

kneth · 2024-02-22T08:31:41Z

packages/fetch/tsconfig.node.esm.json

@@ -2,7 +2,8 @@
  "extends": "./tsconfig.node.base.json",
  "compilerOptions": {
    "outDir": "./dist/node-esm",
-    "module": "ES2022"
+    "module": "es2022",


So you want to prepare for top-level awaits?

kneth · 2024-02-22T08:32:45Z

packages/fetch/tsconfig.node.esm.json

@@ -2,7 +2,8 @@
  "extends": "./tsconfig.node.base.json",
  "compilerOptions": {
    "outDir": "./dist/node-esm",
-    "module": "ES2022"
+    "module": "es2022",
+    "moduleResolution": "Bundler"


According to the docs, it should be bundler

Either is fine, this was suggested by VSCode - it seems this property isn't case-sensitive.

…to injection (#6524) * Making path-browserify an actual dependency * Adding a list of ignored props that can be accessed on the binding, before it's injected. Using Object.assign instead of replacing the injected. * Applying patch before injecting * Adding a note to the changelog * Fixed the injection assertion

) * Adding a "build" script to SDK using TS project references * Removed "realm:bundle" in favour of "realm:build" * Downgrading "module" of shared code to es2015 * Fixing up unit tests * Exporting "bare" types from @realm/fetch * Building only commonjs for now * WIP * No named exports only cjs * Moving public types into dist * Marking toFetchArgs as @internal * Minor changes to platforms * Stop generating TS via Cmake * Seperating "node-wrapper" bindgen into two separate templates reusing a base * Injecting binding instead of importing "realm/binding" * Adding a script to clean the SDK package * No need to emit the native.d.ts in the platform directories anymore * Moved patching of binding to a function called after injection * Fixed relative paths in tsconfig.json * Adding documentation on applyPatch * Emitting source-maps * Renaming "build" script to "build:typescript" * Moved deprecated global Realm into its own file * Refactored namespace API * Renamed "build" to "build:typescript" across depending packages * Expose Auth on the namespace * Renamed "realm:build:typescript" to "realm:build:ts" * Waypoint * Adding a "check-types" script * Adding @internal tags * Exposing Realm on the Realm namespace to help ease ESM transition * Adding a comment * Removed the tsconfig.base.json * Removed an extraneous internal tag * Adding transpilation as a dep of "test" * Fixed running tests and building from scratch * Fixed tests (again) * Fixed missing import in @realm/react * Exporting the namespace deprecated-global.ts * Fixed test coverage * Removed the build:ts:instrumented script again * Adding binding sourcecode to the dist artifact upload * Fixing namespace errors * Fixed lint error * Adding a Jest wrapper for the RN entrypoint * Attempt at fixing coverage * Incorporating feedback * Fixed tsconfigs * Adding binding files to package files * Fixed output of bindgen:generate:typescript

…rior to injection (realm#6524) * Making path-browserify an actual dependency * Adding a list of ignored props that can be accessed on the binding, before it's injected. Using Object.assign instead of replacing the injected. * Applying patch before injecting * Adding a note to the changelog * Fixed the injection assertion

kraenhansen added 10 commits February 13, 2024 21:40

Adding a "build" script to SDK using TS project references

6e88c2d

Removed "realm:bundle" in favour of "realm:build"

29bd876

Downgrading "module" of shared code to es2015

27c977d

Fixing up unit tests

89d60b9

Exporting "bare" types from @realm/fetch

f7a1e68

Building only commonjs for now

34f67f0

WIP

a896a42

No named exports only cjs

37665ed

Moving public types into dist

6ed76e1

Marking toFetchArgs as @internal

abae54d

kraenhansen self-assigned this Feb 16, 2024

cla-bot bot added the cla: yes label Feb 16, 2024

kraenhansen added 18 commits February 16, 2024 22:16

Minor changes to platforms

a842394

Stop generating TS via Cmake

3f8591a

Seperating "node-wrapper" bindgen into two separate templates reusing…

1e9db90

… a base

Injecting binding instead of importing "realm/binding"

e7ae6ea

Adding a script to clean the SDK package

d64d7a5

No need to emit the native.d.ts in the platform directories anymore

61f96bd

Moved patching of binding to a function called after injection

a5ebaeb

Fixed relative paths in tsconfig.json

ceedf35

Adding documentation on applyPatch

282ba72

Emitting source-maps

297982a

Renaming "build" script to "build:typescript"

2e7d5c7

Moved deprecated global Realm into its own file

03feb47

Refactored namespace API

3190aa3

Renamed "build" to "build:typescript" across depending packages

bfeb902

Expose Auth on the namespace

af2f46e

Renamed "realm:build:typescript" to "realm:build:ts"

a29fb4c

Waypoint

c03de59

Adding a "check-types" script

0f14343

kraenhansen marked this pull request as draft February 21, 2024 22:12

kraenhansen added 2 commits February 21, 2024 23:23

Adding transpilation as a dep of "test"

26f5586

Fixed running tests and building from scratch

a0a6971

kraenhansen force-pushed the kh/ts-project-references branch from 34c43bd to a0a6971 Compare February 21, 2024 23:14

kraenhansen added 3 commits February 22, 2024 00:25

Fixed tests (again)

769b0a0

Fixed missing import in @realm/react

429c64f

Exporting the namespace deprecated-global.ts

6c84b75

elle-j approved these changes Feb 22, 2024

View reviewed changes

kraenhansen added 4 commits February 22, 2024 13:14

Fixed test coverage

5574984

Removed the build:ts:instrumented script again

4cd729c

Adding binding sourcecode to the dist artifact upload

0f012aa

Fixing namespace errors

7b16a04

kneth reviewed Feb 22, 2024

View reviewed changes

kraenhansen added 7 commits February 22, 2024 13:54

Fixed lint error

6309190

Adding a Jest wrapper for the RN entrypoint

09aa48d

Attempt at fixing coverage

08dc7a7

Incorporating feedback

d02a472

Fixed tsconfigs

dc6a89b

Adding binding files to package files

e6818c5

Fixed output of bindgen:generate:typescript

5d830f0

kraenhansen marked this pull request as ready for review February 24, 2024 13:43

kraenhansen merged commit 7a06f6c into main Feb 26, 2024
31 checks passed

kraenhansen deleted the kh/ts-project-references branch February 26, 2024 08:48

kraenhansen mentioned this pull request Feb 26, 2024

Improving documentation for realm #6508

Merged

elle-j mentioned this pull request Feb 28, 2024

RJS-2680: Implement support for Mixed data type with nested collections #6513

Merged

2 tasks

kneth mentioned this pull request Mar 20, 2024

Jest test: "Cannot find module 'realm'" #6562

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace "rollup" bundling with TypeScript project references #6492

Replace "rollup" bundling with TypeScript project references #6492

kraenhansen commented Feb 16, 2024 •

edited

elle-j left a comment

elle-j Feb 22, 2024

elle-j Feb 22, 2024

elle-j Feb 22, 2024

elle-j Feb 22, 2024

elle-j Feb 22, 2024

kraenhansen Feb 23, 2024

elle-j Feb 22, 2024

elle-j Feb 22, 2024

elle-j Feb 22, 2024

kraenhansen Feb 23, 2024

elle-j Feb 23, 2024

elle-j Feb 22, 2024

kraenhansen Feb 23, 2024

elle-j Feb 23, 2024

elle-j Feb 22, 2024

kraenhansen Feb 23, 2024

elle-j Feb 23, 2024

kneth left a comment

kneth Feb 22, 2024

kneth Feb 22, 2024

kneth Feb 22, 2024

kraenhansen Feb 22, 2024

		// eslint-disable-next-line @typescript-eslint/no-unused-vars
		export function generate({ spec: boundSpec, file }: TemplateContext, out: Outputter): void {

		mongoClient(serviceName: string): MongoDB {
		mongoClient(serviceName: string): MongoDBService {

	* NOTE: This should only be called after the binding has been injected.
	* @note This should only be called after the binding has been injected.

		import JsPlatformHelpers = binding.JsPlatformHelpers;
		import Helpers = binding.Helpers;

Replace "rollup" bundling with TypeScript project references #6492

Replace "rollup" bundling with TypeScript project references #6492

Conversation

kraenhansen commented Feb 16, 2024 • edited

What, How & Why?

Considerations

No longer including @realm/fetch

We're using stripInternal

☑️ ToDos

elle-j 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

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

Choose a reason for hiding this comment

kneth 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

kraenhansen commented Feb 16, 2024 •

edited

No longer including `@realm/fetch`

We're using `stripInternal`