fix(browser-repl)!: keep operation in progress COMPASS-8576 MONGOSH-1966 #2284

lerouxb · 2024-11-28T13:01:22Z

Changes:

output, history and isOperationInProgress are now all controlled props. So initialOutput and initialHistory was removed.
I took the chance to rewrite Shell to be a function component. So I also converted the tests to use testing-library.

lerouxb · 2024-11-28T13:06:34Z

packages/browser-repl/package.json

@@ -35,7 +35,8 @@
    "depcheck": "depcheck",
    "compile": "tsc -p tsconfig.json",
    "prettier": "prettier",
-    "reformat": "npm run prettier -- --write . && npm run eslint --fix"
+    "reformat": "npm run prettier -- --write . && npm run eslint --fix",
+    "sync-to-compass": "node scripts/sync-to-compass.js"


Useful in combination with this. Based on https://github.com/mongodb-js/compass/blob/main/packages/compass-web/scripts/sync-dist-to-mms.js

This way you can save a file in browser-repl in mongosh and it hot-reloads in compass.

packages/browser-repl/src/components/shell-input.tsx

packages/browser-repl/src/components/shell.tsx

packages/browser-repl/scripts/sync-to-compass.js

kraenhansen · 2024-12-05T07:46:39Z

Just to clarify, is there a reason a simple npm link won't work for this use-case? It seems to me it's serving the same purpose (keeping cross-repo dependencies updated while developing).

lerouxb · 2024-12-05T11:20:41Z

@kraenhansen I don't have a better explanation than https://github.com/mongodb-js/compass/blob/117c32f8ddd5d2bf4f8fceec5b324bf9866de66e/packages/compass-web/scripts/sync-dist-to-mms.js#L47-L52

I don't think any of us have managed to get that work. This is by far the closest to "it just works, instantly and reliably" that I've managed.

(More and more I'm thinking we should just move this code to Compass)

packages/browser-repl/scripts/sync-to-compass.js

packages/browser-repl/src/components/shell-input.tsx

packages/browser-repl/src/components/shell.tsx

Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>

Anemy

Some small things I noticed initially, trying it out now, and taking a look at the usage in Compass.
Nice!

packages/browser-repl/src/components/shell.tsx

packages/browser-repl/README.md

packages/browser-repl/src/components/shell.tsx

gribnoysup · 2024-12-12T10:56:46Z

packages/browser-repl/src/components/shell.tsx

-      await this.updateShellPrompt();
-      this.props.onOperationEnd();
-    }
+  const [editor, setEditor] = useState<EditorRef | null>(null);


I don't think there is any particular reason this should be a state instead of a ref? Using a ref would be more idiomatic here

I changed this now. Does have the problem where it isn't set in time for the useEffect in compass, though.

packages/browser-repl/src/components/shell.tsx

gribnoysup · 2024-12-12T10:58:50Z

packages/browser-repl/src/components/shell.tsx

+  const editorChanged = useCallback(
+    (editor: EditorRef | null) => {
+      setEditor(editor);
+      onEditorChanged?.(editor);


This works, but idiomatically this should probably be done by using a forwarded ref instead of a prop callback

Should be good now?

packages/browser-repl/src/components/shell.tsx

packages/browser-repl/src/sandbox.tsx

…uate

gribnoysup · 2024-12-13T10:02:33Z

packages/browser-repl/src/components/shell.tsx


-    this.setState({ output });
-  }
+  const [isFirstRun, setIsFirstRun] = useState(true);


Just a nit / fyi, I think it's okay to keep like that also: this works, but causes unnecessary re-renders. As we discussed, usually anything that is not directly related to rendering should be a ref (or all deps of the effect should be made into refs to avoid dependencies here, depends on what flavor of weird React workarounds you would have a preference for). You'd usually want to limit the amount of unnecessary updates your component will do, performance aside, it just means that you have less cascading side-effects that you might not expect otherwise

Like this? 4e66b19

Don't know why I didn't consider just updating the ref. This stuff still isn't muscle memory for me - somehow I still don't do enough react often enough.

There are tons of improvements we can make in future, though. Maybe easier once we move this to Compass.

lerouxb added 2 commits November 28, 2024 12:59

Don't lose the isOperationInProgress state when the user switches tabs

a339feb

minus comment

6e7d247

lerouxb mentioned this pull request Nov 28, 2024

fix(compass-shell): Don't lose the isOperationInProgress state when the user switches tabs, bump mongosh to 2.3.6 COMPASS-8576 COMPASS-8689 mongodb-js/compass#6538

Merged

lerouxb commented Nov 28, 2024

View reviewed changes

packages/browser-repl/src/components/shell-input.tsx Outdated Show resolved Hide resolved

lerouxb changed the title ~~chore(browser-repl): Keep operation in progress COMPASS-8576~~ fix(browser-repl): Keep operation in progress COMPASS-8576 Nov 28, 2024

addaleax changed the title ~~fix(browser-repl): Keep operation in progress COMPASS-8576~~ fix(browser-repl): keep operation in progress COMPASS-8576 Nov 28, 2024

addaleax reviewed Nov 28, 2024

View reviewed changes

packages/browser-repl/src/components/shell.tsx Outdated Show resolved Hide resolved

lerouxb added 5 commits December 4, 2024 11:56

initial like the others for now

98430ee

progress towards moving Shell to be a function component

4a95d91

typo

5d48859

readme tweaks

8587f0d

rm comment

2e85187

kraenhansen reviewed Dec 5, 2024

View reviewed changes

packages/browser-repl/scripts/sync-to-compass.js Show resolved Hide resolved

lerouxb added 2 commits December 6, 2024 16:50

browser-repl tests using testing library

8ca4f52

Merge branch 'main' into keep-operation-in-progress2

5ab819b

addaleax reviewed Dec 9, 2024

View reviewed changes

lerouxb and others added 8 commits December 9, 2024 15:28

Update packages/browser-repl/scripts/sync-to-compass.js

45946cb

Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>

Update packages/browser-repl/scripts/sync-to-compass.js

4a7f4e9

Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>

Update packages/browser-repl/src/components/shell.tsx

5cf7a44

Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>

Update packages/browser-repl/src/components/shell.tsx

e3bf5f7

Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>

Update packages/browser-repl/src/components/shell.tsx

a87e420

Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>

always clear on enter

85143c0

lint

e54962a

Merge branch 'main' into keep-operation-in-progress2

50313a7

Anemy reviewed Dec 11, 2024

View reviewed changes

packages/browser-repl/src/components/shell.tsx Outdated Show resolved Hide resolved

packages/browser-repl/README.md Outdated Show resolved Hide resolved

packages/browser-repl/README.md Show resolved Hide resolved

packages/browser-repl/src/components/shell.tsx Outdated Show resolved Hide resolved

lerouxb added 2 commits December 11, 2024 16:54

remove darkMode

7c3a692

move the eslint comments

e24af9f

lerouxb added 4 commits December 11, 2024 16:56

update readme

3567fcb

Merge branch 'main' into keep-operation-in-progress2

705f4ff

keep the text value as initialText

be07c02

and the sandbox

686cbc9

gribnoysup reviewed Dec 12, 2024

View reviewed changes

lerouxb added 7 commits December 12, 2024 11:46

use shell input container ref

b34b9a4

cap history before redacting

757ef7c

unnecessary lint comment

72d16b3

editor ref

72a438b

test fixes

d0b920d

unnecessary import

9b15c06

more stable

add38a0

Anemy reviewed Dec 12, 2024

View reviewed changes

packages/browser-repl/src/sandbox.tsx Show resolved Hide resolved

lerouxb added 2 commits December 12, 2024 22:14

don't re-run initialEvaluate AND don't lose the output

6f20f37

optimisation: don't get the shell prompt before executing initialEval…

37e3c8b

…uate

gribnoysup approved these changes Dec 13, 2024

View reviewed changes

use refs and update them

4e66b19

gribnoysup approved these changes Dec 13, 2024

View reviewed changes

lerouxb merged commit 1e43c79 into main Dec 13, 2024
118 of 123 checks passed

lerouxb deleted the keep-operation-in-progress2 branch December 13, 2024 13:34

lerouxb changed the title ~~fix(browser-repl): keep operation in progress COMPASS-8576~~ fix(browser-repl)!: keep operation in progress COMPASS-8576 Dec 13, 2024

lerouxb changed the title ~~fix(browser-repl)!: keep operation in progress COMPASS-8576~~ fix(browser-repl)!: keep operation in progress COMPASS-8576 MONGOSH-1966 Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(browser-repl)!: keep operation in progress COMPASS-8576 MONGOSH-1966 #2284

fix(browser-repl)!: keep operation in progress COMPASS-8576 MONGOSH-1966 #2284

lerouxb commented Nov 28, 2024 •

edited

Loading

lerouxb Nov 28, 2024

kraenhansen commented Dec 5, 2024

lerouxb commented Dec 5, 2024 •

edited

Loading

Anemy left a comment

gribnoysup Dec 12, 2024

lerouxb Dec 12, 2024

gribnoysup Dec 12, 2024

lerouxb Dec 12, 2024

gribnoysup Dec 13, 2024

lerouxb Dec 13, 2024

lerouxb Dec 13, 2024 •

edited

Loading

fix(browser-repl)!: keep operation in progress COMPASS-8576 MONGOSH-1966 #2284

fix(browser-repl)!: keep operation in progress COMPASS-8576 MONGOSH-1966 #2284

Conversation

lerouxb commented Nov 28, 2024 • edited Loading

lerouxb Nov 28, 2024

Choose a reason for hiding this comment

kraenhansen commented Dec 5, 2024

lerouxb commented Dec 5, 2024 • edited Loading

Anemy left a comment

Choose a reason for hiding this comment

gribnoysup Dec 12, 2024

Choose a reason for hiding this comment

lerouxb Dec 12, 2024

Choose a reason for hiding this comment

gribnoysup Dec 12, 2024

Choose a reason for hiding this comment

lerouxb Dec 12, 2024

Choose a reason for hiding this comment

gribnoysup Dec 13, 2024

Choose a reason for hiding this comment

lerouxb Dec 13, 2024

Choose a reason for hiding this comment

lerouxb Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

lerouxb commented Nov 28, 2024 •

edited

Loading

lerouxb commented Dec 5, 2024 •

edited

Loading

lerouxb Dec 13, 2024 •

edited

Loading