Make changes required for Desktop FS updates #1099

SleepyLeslie · 2024-07-10T20:11:46Z

Implements required changes for Desktop user experience overhaul grist-desktop#42
Adds a --no-nodemon flag to yarn start that allows grist-desktop to use the script without running nodemon.
Updates use of deprecated TypeORM API.
Makes a new class MergedServer that does not run the server automatically on creation. This allows for modifying certain aspects of the MergedServer before running it.
Moves login system from ServerOptions to ICreate.

app/server/MergedServer.ts

app/server/lib/FlexServer.ts

app/server/lib/ICreate.ts

test/gen-server/AuthCaching.ts

paulfitz · 2024-07-26T18:48:04Z

app/client/ui/HomeLeftPane.ts

@@ -24,6 +21,7 @@ import {Workspace} from 'app/common/UserAPI';
 import {computed, dom, domComputed, DomElementArg, observable, Observable, styled} from 'grainjs';
 import {createHelpTools, cssLeftPanel, cssScrollPane,
        cssSectionHeader, cssTools} from 'app/client/ui/LeftPanelCommon';
+import {createDocAndOpen, importDocAndOpen, importFromPluginAndOpen} from "app/client/ui/NewDocMethods";


Please use locally consistent quotes. Please do your best to put imports in alphabetical order, even if prev devs have left small muddles.

I reordered all imports here manually.

I noticed that our eslint config has ignoreDeclarationSort: true, which effectively disables sort-imports checks across lines. Is that intentional?

paulfitz · 2024-07-26T18:51:13Z

stubs/app/server/lib/logins.ts

@@ -1,6 +0,0 @@
-import { getCoreLoginSystem } from "app/server/lib/coreLogins";


(We will need to be careful when syncing SaaS, also grist-static, since they are still using logins.ts)

stubs/app/client/ui/HomeImports.ts

paulfitz · 2024-07-31T13:48:49Z

stubs/app/client/ui/NewDocMethods.ts

+import {ImportSourceElement} from 'app/client/lib/ImportSourceElement';
+import {reportError} from 'app/client/models/AppModel';
+
+export async function createDocAndOpen(home: HomeModel) {


Adding significant new code to stubs is undesirable, unless it is for grist-core and only grist-core, not desktop, not ee, not saas etc.

I do have a doubt that maybe desktop is using some material from stubs? But if so ideally that material would go into core/ and be used from desktop ext and by stubs.

Desktop is using version.ts from stubs. I just removed it. Now there is no core/stubs dependency.

SleepyLeslie · 2024-08-02T21:25:00Z

app/server/MergedServer.ts

+  public hasComponent(serverType: ServerType) {
+    return this._serverTypes.includes(serverType);
+  }


This replaces includeHome, includeDocs, includeStatic and includeApp.

SleepyLeslie · 2024-08-02T21:25:54Z

app/server/MergedServer.ts

+  externalStorage?: boolean;
+}
+
+export class MergedServer {


A new class that holds all necessary information such that the MergedServer can be created and started as separate steps.

SleepyLeslie · 2024-08-02T21:27:22Z

app/server/MergedServer.ts

+    } else {
+      ms.flexServer.setServesPlugins(false);
+    }
+


Code to handle options.loginSystem is removed because login system has been moved to ICreate. FlexServer will now deal with the login system.

SleepyLeslie · 2024-08-02T21:28:57Z

app/server/MergedServer.ts

+
+export class MergedServer {
+
+  public static async create(port: number, serverTypes: ServerType[], options: ServerOptions = {}) {


Because the constructor for MergedServer has to be async, I used this static function. It contains everything in the old main function up to server.start().

SleepyLeslie · 2024-08-02T21:30:32Z

app/server/MergedServer.ts

+  }
+
+
+  public async run() {


This is the second part of the old main function.

The FlexServer is created in create(), but before calling run() it is not started. Between these two function calls, one can retrieve and modify certain FlexServer components.

SleepyLeslie · 2024-08-02T21:44:29Z

app/server/MergedServer.ts

+  // If set, documents saved to external storage such as s3 (default is to check environment variables,
+  // which get set in various ways in dev/test entry points)
+  externalStorage?: boolean;
+}


loginSystem is removed because it is now specified via ICreate instead.

The testing hooks were registering before the _comm port had been created. This adjusts the startup order to allow the server to be initialised before the testing hooks are registered.

Spoffy · 2024-08-13T19:15:16Z

app/server/lib/ICreate.ts

+  getLoginSystem(): Promise<GristLoginSystem>;
+
+  // Used by Grist Desktop to override the getPath function.
+  decorateDocStorageManager? (original: IDocStorageManager): void;


I'm a bit concerned about having this here, with the intended purpose of overriding methods from inside an arbitrary interface. It feels like it's asking for unexpected trouble in the future.

Refactored this in #1180 to allow a new storage manager to be injected

Spoffy · 2024-09-11T13:08:30Z

app/client/ui/AppUI.ts

+  // Expose the HomeModel for grist-desktop.
+  (window as any).gristHomeModel = pageModel;


I suspect we can remove this by using window.gristApp instead.

This replaces decorateDocStorageManager with new ICreate methods that allow the DocStorageManager implementation to overridden by other versions of Grist, allowing them to subclass or use a custom implementation.

paulfitz

It all makes reasonable sense. I'd guess this will break SaaS and we'll need to scramble to fix it (loginSystem changes, storageManager creation changes, new stubs to define), but that is normal these days.

@Spoffy could you run this PR through the SaaS CI just to see what the damage is?

paulfitz · 2024-09-11T14:36:04Z

app/server/devServerMain.ts

@@ -118,17 +119,18 @@ export async function main() {
  log.info("== staticServer");
  const staticPort = getPort("STATIC_PORT", 9001);
  process.env.APP_STATIC_URL = `http://localhost:${staticPort}`;
-  await mergedServerMain(staticPort, ["static"]);
+  await MergedServer.create(staticPort, ["static"]).then((s) => s.run());


weak preference for spelling this out with an intermediate variable rather than using a then

paulfitz · 2024-09-11T14:43:10Z

app/server/lib/ICreate.ts

@@ -12,6 +13,11 @@ import {IShell} from 'app/server/lib/IShell';
 import {createSandbox, SpawnFn} from 'app/server/lib/NSandbox';
 import {SqliteVariant} from 'app/server/lib/SqliteCommon';
 import {ITelemetry} from 'app/server/lib/Telemetry';
+import {IDocStorageManager} from './IDocStorageManager';


We have a near-universal convention of spelling out the paths as app/.... Not much reason for it, but grist-static does depend on it in some cases so it would be nice to keep.

paulfitz · 2024-09-11T14:45:30Z

test/server/docTools.ts

@@ -17,6 +16,7 @@ import * as fse from 'fs-extra';
 import {tmpdir} from 'os';
 import * as path from 'path';
 import * as tmp from 'tmp';
+import {create} from "app/server/lib/create";


import ordering

# Conflicts: # .gitignore # app/client/ui/HomeLeftPane.ts # app/gen-server/lib/homedb/HomeDBManager.ts # app/server/lib/FlexServer.ts

paulfitz

There are some unresolved review comments, but all minor, and this version has been checked on SaaS and the fixes needed there are figured out. So going to merge. Thanks @SleepyLeslie @Spoffy.

Make a set of changes required for Desktop FS improvements, see gristlabs/grist-desktop#42 --------- Co-authored-by: Spoffy <[email protected]> Co-authored-by: Spoffy <[email protected]>

SleepyLeslie mentioned this pull request Jul 11, 2024

Desktop user experience overhaul gristlabs/grist-desktop#42

Merged

SleepyLeslie force-pushed the sleepyleslie/nativefs branch 4 times, most recently from 06eb1c2 to 140eb4f Compare July 11, 2024 18:26

SleepyLeslie added 2 commits July 15, 2024 17:30

Make changes required for Desktop FS updates

fc9f993

Move DocStorageManager decorator to ICreate

701e187

SleepyLeslie force-pushed the sleepyleslie/nativefs branch 2 times, most recently from c0a0d09 to ab32eac Compare July 17, 2024 15:41

SleepyLeslie added 4 commits July 17, 2024 18:19

Rename docStorageManagerDecorator

189f724

Fix --no-nodemon in watch script

6eac878

Stub NewDocMethods

e5b5f23

Update gitignore

9f60f25

SleepyLeslie force-pushed the sleepyleslie/nativefs branch from ab32eac to 9f60f25 Compare July 18, 2024 16:21

SleepyLeslie added 2 commits July 18, 2024 12:26

Make linter happy

7f7b18f

Move login system into ICreate

e5b47d6

paulfitz reviewed Jul 22, 2024

View reviewed changes

app/server/MergedServer.ts Show resolved Hide resolved

SleepyLeslie force-pushed the sleepyleslie/nativefs branch from a8ef6a8 to 3505b30 Compare July 26, 2024 14:54

Make changes for the "Import Document" button

9de01df

SleepyLeslie force-pushed the sleepyleslie/nativefs branch from 3505b30 to 9de01df Compare July 26, 2024 17:04

paulfitz reviewed Jul 26, 2024

View reviewed changes

SleepyLeslie added 2 commits July 26, 2024 19:05

Expose gristHomeModel to window

fd4f048

Fix linting issues

60f0442

paulfitz reviewed Jul 31, 2024

View reviewed changes

SleepyLeslie commented Aug 2, 2024

View reviewed changes

SleepyLeslie added 2 commits August 2, 2024 18:18

Merge branch 'main' into sleepyleslie/nativefs

a6fbe65

Merge branch 'main' into sleepyleslie/nativefs

9950120

SleepyLeslie force-pushed the sleepyleslie/nativefs branch from 89089b0 to 9950120 Compare August 5, 2024 22:21

SleepyLeslie and others added 3 commits August 6, 2024 01:47

Fix server tests

449a53b

Fixes testing hooks registering too early

6fa0857

The testing hooks were registering before the _comm port had been created. This adjusts the startup order to allow the server to be initialised before the testing hooks are registered.

Make new stubs actual simple stubs

2f84093

Spoffy reviewed Aug 13, 2024

View reviewed changes

Spoffy mentioned this pull request Aug 26, 2024

Spoffy/nativefs updates #1180

Merged

4 tasks

Spoffy reviewed Sep 11, 2024

View reviewed changes

Spoffy and others added 2 commits September 11, 2024 14:35

Replaces decorateDocStorageManager in ICreate

9fbbdc3

This replaces decorateDocStorageManager with new ICreate methods that allow the DocStorageManager implementation to overridden by other versions of Grist, allowing them to subclass or use a custom implementation.

Removes window.gristHomeModel

afcb3de

paulfitz reviewed Sep 11, 2024

View reviewed changes

Spoffy added 4 commits September 13, 2024 16:51

Merge branch 'main' into sleepyleslie/nativefs

87d1d0a

# Conflicts: # .gitignore # app/client/ui/HomeLeftPane.ts # app/gen-server/lib/homedb/HomeDBManager.ts # app/server/lib/FlexServer.ts

Adds missing semicolon

9d53e30

Fixes tests using mergedServerMain

0030a82

Merge branch 'main' into sleepyleslie/nativefs

947170d

Spoffy force-pushed the sleepyleslie/nativefs branch from 01e3fc5 to 947170d Compare September 16, 2024 21:52

paulfitz approved these changes Sep 17, 2024

View reviewed changes

paulfitz merged commit 02cfcee into main Sep 17, 2024
11 checks passed

paulfitz deleted the sleepyleslie/nativefs branch September 17, 2024 01:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make changes required for Desktop FS updates #1099

Make changes required for Desktop FS updates #1099

SleepyLeslie commented Jul 10, 2024 •

edited

Loading

paulfitz Jul 26, 2024

SleepyLeslie Jul 30, 2024

paulfitz Jul 26, 2024

paulfitz Jul 31, 2024

paulfitz Jul 31, 2024

SleepyLeslie Aug 7, 2024

SleepyLeslie Aug 2, 2024

SleepyLeslie Aug 2, 2024

SleepyLeslie Aug 2, 2024

SleepyLeslie Aug 2, 2024

SleepyLeslie Aug 2, 2024

SleepyLeslie Aug 2, 2024

Spoffy Aug 13, 2024

Spoffy Aug 31, 2024

Spoffy Sep 11, 2024

paulfitz left a comment

paulfitz Sep 11, 2024

paulfitz Sep 11, 2024

paulfitz Sep 11, 2024

paulfitz left a comment

		@@ -1,6 +0,0 @@
		import { getCoreLoginSystem } from "app/server/lib/coreLogins";


		export class MergedServer {

		public static async create(port: number, serverTypes: ServerType[], options: ServerOptions = {}) {

		// Expose the HomeModel for grist-desktop.
		(window as any).gristHomeModel = pageModel;

Make changes required for Desktop FS updates #1099

Make changes required for Desktop FS updates #1099

Conversation

SleepyLeslie commented Jul 10, 2024 • 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

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

paulfitz 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

paulfitz left a comment

Choose a reason for hiding this comment

SleepyLeslie commented Jul 10, 2024 •

edited

Loading