Feat: Add Prettier Support to TypeScript Notebook #231

swk777 · 2024-08-30T04:10:48Z

implement #209

Implement server-side code formatting using Prettier
Add a button and hotkey for triggering code formatting
Support customizable Prettier configurations

Minor Details
• Upgraded Radix UI to include the brush icon
• Moved Prettier configuration to package.json since having a standalone configuration doesn’t seem crucial at this point.
• Set Command+Shift+F as the shortcut for formatting.

versecafe · 2024-08-30T05:47:01Z

Should this be debounced auto or a manual format button & key bind to avoid changing the lines around as someone works if they pause between typing?

versecafe · 2024-08-30T05:49:24Z

Could this be done on the server to avoid making the client bundle even heavier and avoid the need to send the entire formatted file down to the server, change the fs, and then sync back with the client

swk777 · 2024-08-30T07:03:29Z

Could this be done on the server to avoid making the client bundle even heavier and avoid the need to send the entire formatted file down to the server, change the fs, and then sync back with the client

It’s true, but the lengthy process isn’t ideal for near real-time auto-formatting, and the sync model is currently not supported.

benjreinhart

Thank you for this!

TLDR: implementation looks great but I'm not sure this is the UX we want.

I think having code shift out from under me after a slight pause in typing is not ideal. In my editor it formats only on save which is an explicit action I take. In Srcbook, we don't explicitly save, so that won't work here. However, I think we can apply the same principle and use a keyboard shortcut and/or icon as the explicit action for formatting. This way I opt-in to each format which I feel pretty strongly about.
If we use a keyboard shortcut and/or icon, then we don't need the database entry for auto format.
I'm torn on the client-side vs server-side formatting functionality. I would prefer it to take place inside the folder we create for the Srcbook (similar to how we handle tsconfig and type checking). That is, I would expect that repository to have prettier installed in its deps and the prettier config to be located in that folder as well. In theory, this makes ejecting a srcbook folder a breeze, which is something I think we'll eventually want to do. BUT, if the user presses the format icon and there's a delay I agree that's a poor experience. I wonder if it's fast enough there's no real delay?
It's ok if we do it in a follow up PR, but the prettier settings should be editable by the user, the same as package.json or tsconfig.json.

Excellent work on implementation here but I think we should change the UX

benjreinhart · 2024-08-30T18:29:54Z

packages/web/src/components/cells/code.tsx

+                    onClick={onFormatCode}
+                    tabIndex={1}
+                  >
+                    <CodeXml size={16} />


Couple notes on this:

I wonder if there's a common icon for code formatting? This icon is ambiguous and could be used to indicate many different things in a product like ours. Maybe braces, but that doesn't feel much different?

This icon feels a bit odd with the current debounce UX. You have < 2 seconds after typing anything before pressing this icon will have no affect. That said, it is useful if you import a srcbook that isn't formatted according to your settings and want to format the cell without typing in it first.

I like, and I think I've seen this before, using a brush icon:

PaintbrushVertical in lucide-react.

benjreinhart · 2024-08-30T18:30:39Z

packages/web/src/components/cells/code.tsx

@@ -643,6 +680,27 @@ function CodeEditor({

  const updateCellOnServerDebounced = useDebouncedCallback(updateCellOnServer, DEBOUNCE_DELAY);

+  const formatCellCode = async (source: string, viewUpdate: ViewUpdate) => {


Nit: There's no await inside this function so we don't need the async keyword in this case. Alternatively can change to using await and ditch the promise callbacks.

versecafe · 2024-08-30T18:53:37Z

@swk777 @benjreinhart On the server driven delay the back and forth with the WS in local is ~50ms (hardware dependent) under the current system including validation with zod, so it should not create any sort of noticeable delay to a user, this may be different for a hosted instance but even a 500ms delay if you play a little animation isn't an issue since the format is something you do maybe once or twice when working on a cell.

nichochar

Thanks for the contrib @swk777

I apologize, I think the ticket should have been more specific on what we wanted, but I agree with Ben's review. I think a better behavior is to implement the formatting on the backend side, with the ability to trigger it from the frontend. So:

on each cell, you can click a button in the action bar (PaintbrushVertical) which formats it by calling a websocket event CellUpdateFormatCell and updating the cell.source.
[Stretch 1] Auto-format on export: if auto-format is enabled, we format all saves on export only. Otherwise as @benjreinhart says, things change while you're typing and that's a very jarring experience.
[Stretch 2]: add a global keyboard shortcut to format all cells (and/or format current active cell).

nichochar · 2024-09-01T20:35:24Z

packages/web/src/components/cells/code.tsx

+                    onClick={onFormatCode}
+                    tabIndex={1}
+                  >
+                    <CodeXml size={16} />


I like, and I think I've seen this before, using a brush icon:

PaintbrushVertical in lucide-react.

swk777 · 2024-09-02T03:11:56Z

Thank you for the detailed feedback

I’ll dig into implementing the formatting on the server side as suggested.
To keep the scope of this PR manageable, I’d like to start by implementing formatting for individual cells, triggered by a button or keyboard shortcut. We can then use this for a while to see if there’s a real need for global formatting. If it turns out to be necessary, we can easily add it later, as the implementation should be straightforward.

Does this approach sound good to you? @benjreinhart @nichochar

nichochar · 2024-09-02T03:14:35Z

Approach sounds good to me. I would add a button with the paintbrush right here to start:

versecafe · 2024-09-06T17:39:35Z

@swk777 When you're finishing this up can you merge main back in and run pnpm changeset to create the changelog

versecafe · 2024-09-09T03:27:37Z

@swk777 please merge main in and get tests and format checks passing

versecafe · 2024-09-09T17:24:08Z

Should format errors be piped up out to be more visible as the current toast doesn't explain the error but it is nicely captured in the console as

   21 |  */
   22 | export class TicTacToe {
 > 23 |   private board: string[[];
      |                           ^
   24 |   private currentPlayer: string;
   25 |
   26 |   constructor() {

versecafe

Code side all looks good just the note on making errors on format runs include more information on the front end.

swk777 · 2024-09-10T08:20:00Z

Code side all looks good just the note on making errors on format runs include more information on the front end.

It’s true, I’ve now routed the output to stderr. The error does overlap somewhat with the ones in the problems panel, but it currently depends on the TypeScript package installation. Once #210 is completed, I can work on further optimizations.

benjreinhart

This is looking really good, excited for this. Couple last minute items but it's almost good to go. Thank you!

benjreinhart · 2024-09-10T19:51:59Z

packages/web/src/components/cells/code.tsx

+      keymap.of([
+        { key: 'Mod-Enter', run: evaluateModEnter },
+        {
+          key: 'Mod-Shift-f',


Could we use Shift+Option+F for this (this is what vscode uses)? I'm a bit worried about cmd+f and shift+cmd+f because those are universally used for searching a file or searching a project, which we'll probably want at some point.

This just got me thinking should we add srcbook keymap support as a JSON and with the default, vscode, jetbrains, vim keymaps vim may be hard for nav but at least for basic actions

That would be cool but I would say that's out of scope for this PR (not that you're proposing doing it here)

benjreinhart · 2024-09-10T20:04:39Z

packages/api/session.mts

@@ -296,7 +298,69 @@ export function updateCell(session: SessionType, cell: CellType, updates: CellUp
      return updateCodeCell(session, cell, updates);
  }
 }
-
+async function ensurePrettierInstalled(dir: string): Promise<boolean> {


Some thoughts here:

If formatting is going to be a first class citizen in all Srcbooks, then we should create every new srcbook with it already present in the package.json's devDependencies

If we do check for its presence here, are we able to use depcheck? We already use this in deps.ts and it looks like it has "special" support for prettier.

If the dep is missing, I'm inclined to prompt the user for installing it. While this does introduce an extra manual step, it keeps the user in the loop and gives faster feedback than if they waited for install to happen behind the scenes. This should rarely happen if we include prettier in the dependencies when creating new srcbooks, so I think that's acceptable?

I stand behind number 3 above but in the interest of getting this out sooner than later, we can ignore that piece for now and leave this code (though would be good to use depcheck).

Ohh I just noticed we are doing 1) here already, nice

benjreinhart · 2024-09-10T20:05:01Z

packages/api/session.mts

+      try {
+        npmInstall({
+          cwd: dir,
+          packages: ['prettier'],


We should install this in the devDependencies

benjreinhart · 2024-09-10T20:32:32Z

You're going to want to rebase main as well given the navigation updates (#267).

swk777 · 2024-09-11T02:43:23Z

all done, thx

benjreinhart

You'll need to rebase though as #270 touched all the dependency files.

benjreinhart · 2024-09-11T21:50:01Z

.changeset/thin-dots-rest.md

@@ -0,0 +1,7 @@
+---
+'@srcbook/shared': minor


I also just noticed this is minor, but for now I would like to keep all the versions on patch. Do you mind regenerating this changeset such that these are all patch?

you don't need to regen just change the text to patch and it'll work

benjreinhart · 2024-09-12T02:41:33Z

This is awesome, appreciate all your work here! 🙏🏻

nichochar · 2024-09-12T14:27:56Z

Amazing, thanks for the great contribution, really like having it in there, it feels great. After using it a couple of times, I think we'll want to add some indication that it's running (like AI has a purple ring, and running a process has a yellow ring), but we'll ask design for this.

Thanks @swk777 🚀 👊

benjreinhart reviewed Aug 30, 2024

View reviewed changes

versecafe assigned versecafe and unassigned versecafe Aug 31, 2024

nichochar reviewed Sep 1, 2024

View reviewed changes

swk777 force-pushed the prettier branch 2 times, most recently from 24d57ac to 8190fba Compare September 6, 2024 04:22

swk777 force-pushed the prettier branch 2 times, most recently from ddb6a94 to 5298c9c Compare September 9, 2024 02:50

swk777 requested a review from nichochar September 9, 2024 03:11

versecafe linked an issue Sep 9, 2024 that may be closed by this pull request

Support formatting (prettier?) #209

Closed

versecafe approved these changes Sep 9, 2024

View reviewed changes

benjreinhart reviewed Sep 10, 2024

View reviewed changes

swk777 force-pushed the prettier branch from db6ba21 to e720c22 Compare September 11, 2024 02:42

benjreinhart approved these changes Sep 11, 2024

View reviewed changes

benjreinhart reviewed Sep 11, 2024

View reviewed changes

swk777 added 3 commits September 12, 2024 09:20

use prettier command to format code

aa78e91

improve ui interaction

8cf1a1d

remove console

5ed5a39

swk777 added 6 commits September 12, 2024 09:23

refact

dfabe78

use package.json prettier config

800d13c

ensure prettier installed when formatting

36a717c

fix lint & add changelog

34737ea

pipe format error to stderr

0a68c3d

change shortcut & install prettier in dev

a394861

swk777 force-pushed the prettier branch from e720c22 to a394861 Compare September 12, 2024 01:27

remove tmp file

7d436a4

benjreinhart merged commit 545699e into srcbookdev:main Sep 12, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add Prettier Support to TypeScript Notebook #231

Feat: Add Prettier Support to TypeScript Notebook #231

swk777 commented Aug 30, 2024 •

edited

Loading

versecafe commented Aug 30, 2024

versecafe commented Aug 30, 2024

swk777 commented Aug 30, 2024 •

edited

Loading

benjreinhart left a comment

benjreinhart Aug 30, 2024 •

edited

Loading

nichochar Sep 1, 2024

benjreinhart Aug 30, 2024

versecafe commented Aug 30, 2024

nichochar left a comment •

edited

Loading

nichochar Sep 1, 2024

swk777 commented Sep 2, 2024

nichochar commented Sep 2, 2024

versecafe commented Sep 6, 2024

versecafe commented Sep 9, 2024

versecafe commented Sep 9, 2024

versecafe left a comment

swk777 commented Sep 10, 2024

benjreinhart left a comment

benjreinhart Sep 10, 2024

versecafe Sep 10, 2024

benjreinhart Sep 10, 2024

benjreinhart Sep 10, 2024 •

edited

Loading

benjreinhart Sep 10, 2024 •

edited

Loading

benjreinhart Sep 10, 2024 •

edited

Loading

benjreinhart Sep 10, 2024

benjreinhart commented Sep 10, 2024

swk777 commented Sep 11, 2024

benjreinhart left a comment

benjreinhart Sep 11, 2024

versecafe Sep 11, 2024

benjreinhart commented Sep 12, 2024

nichochar commented Sep 12, 2024

		@@ -643,6 +680,27 @@ function CodeEditor({

		const updateCellOnServerDebounced = useDebouncedCallback(updateCellOnServer, DEBOUNCE_DELAY);

		const formatCellCode = async (source: string, viewUpdate: ViewUpdate) => {

Feat: Add Prettier Support to TypeScript Notebook #231

Feat: Add Prettier Support to TypeScript Notebook #231

Conversation

swk777 commented Aug 30, 2024 • edited Loading

versecafe commented Aug 30, 2024

versecafe commented Aug 30, 2024

swk777 commented Aug 30, 2024 • edited Loading

benjreinhart left a comment

Choose a reason for hiding this comment

benjreinhart Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

versecafe commented Aug 30, 2024

nichochar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swk777 commented Sep 2, 2024

nichochar commented Sep 2, 2024

versecafe commented Sep 6, 2024

versecafe commented Sep 9, 2024

versecafe commented Sep 9, 2024

versecafe left a comment

Choose a reason for hiding this comment

swk777 commented Sep 10, 2024

benjreinhart 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

benjreinhart Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

benjreinhart Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

benjreinhart Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjreinhart commented Sep 10, 2024

swk777 commented Sep 11, 2024

benjreinhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjreinhart commented Sep 12, 2024

nichochar commented Sep 12, 2024

swk777 commented Aug 30, 2024 •

edited

Loading

swk777 commented Aug 30, 2024 •

edited

Loading

benjreinhart Aug 30, 2024 •

edited

Loading

nichochar left a comment •

edited

Loading

benjreinhart Sep 10, 2024 •

edited

Loading

benjreinhart Sep 10, 2024 •

edited

Loading

benjreinhart Sep 10, 2024 •

edited

Loading