Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Add Prettier Support to TypeScript Notebook #231

Merged
merged 10 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/thin-dots-rest.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@srcbook/shared': minor
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

'@srcbook/api': minor
'@srcbook/web': minor
---

Add Prettier Support to Code Notebook
16 changes: 12 additions & 4 deletions packages/api/exec.mts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export type NodeRequestType = BaseExecRequestType & {

export type NPMInstallRequestType = BaseExecRequestType & {
packages?: Array<string>;
args?: Array<string>;
};

type SpawnCallRequestType = {
Expand Down Expand Up @@ -132,10 +133,17 @@ export function tsx(options: NodeRequestType) {
*/
export function npmInstall(options: NPMInstallRequestType) {
const { cwd, stdout, stderr, onExit } = options;

const args = options.packages
? ['install', '--include=dev', ...options.packages]
: ['install', '--include=dev'];
? ['install', '--include=dev', ...(options.args || []), ...options.packages]
: ['install', '--include=dev', ...(options.args || [])];

return spawnCall({ command: 'npm', cwd, args, stdout, stderr, onExit, env: process.env });
return spawnCall({
command: 'npm',
cwd,
args,
stdout,
stderr,
onExit,
env: process.env,
});
}
73 changes: 59 additions & 14 deletions packages/api/server/ws.mts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
removeCell,
updateCodeCellFilename,
addCell,
formatAndUpdateCodeCell,
} from '../session.mjs';
import { getSecrets } from '../config.mjs';
import type { SessionType } from '../types.mjs';
Expand All @@ -25,6 +26,7 @@ import type {
DepsValidatePayloadType,
CellStopPayloadType,
CellUpdatePayloadType,
CellFormatPayloadType,
TsServerStartPayloadType,
TsServerStopPayloadType,
CellDeletePayloadType,
Expand All @@ -42,6 +44,7 @@ import {
CellUpdatedPayloadSchema,
CellRenamePayloadSchema,
CellDeletePayloadSchema,
CellFormatPayloadSchema,
CellExecPayloadSchema,
CellStopPayloadSchema,
AiGenerateCellPayloadSchema,
Expand All @@ -60,6 +63,7 @@ import {
TsServerCellSuggestionsPayloadSchema,
TsServerQuickInfoRequestPayloadSchema,
TsServerQuickInfoResponsePayloadSchema,
CellFormattedPayloadSchema,
} from '@srcbook/shared';
import tsservers from '../tsservers.mjs';
import { TsServer } from '../tsserver/tsserver.mjs';
Expand Down Expand Up @@ -417,6 +421,43 @@ async function cellFixDiagnostics(payload: AiFixDiagnosticsPayloadType) {
});
}

async function cellFormat(payload: CellFormatPayloadType) {
const session = await findSession(payload.sessionId);
if (!session) {
throw new Error(`No session exists for session '${payload.sessionId}'`);
}
const cellBeforeUpdate = findCell(session, payload.cellId);

if (!cellBeforeUpdate || cellBeforeUpdate.type !== 'code') {
throw new Error(
`No cell exists or not a code cell for session '${payload.sessionId}' and cell '${payload.cellId}'`,
);
}
const result = await formatAndUpdateCodeCell(session, cellBeforeUpdate);
if (!result.success) {
wss.broadcast(`session:${session.id}`, 'cell:output', {
cellId: payload.cellId,
output: { type: 'stderr', data: result.errors },
});
sendCellUpdateError(session, payload.cellId, [
{
message:
'An error occurred while formatting the code. Please check the "stderr" for more details.',
attribute: 'formatting',
},
]);
} else {
const cell = result.cell as CodeCellType;

wss.broadcast(`session:${session.id}`, 'cell:formatted', {
cellId: payload.cellId,
cell,
});

refreshCodeCellDiagnostics(session, cell);
}
}

async function cellUpdate(payload: CellUpdatePayloadType) {
const session = await findSession(payload.sessionId);

Expand All @@ -431,7 +472,6 @@ async function cellUpdate(payload: CellUpdatePayloadType) {
`No cell exists for session '${payload.sessionId}' and cell '${payload.cellId}'`,
);
}

const result = await updateCell(session, cellBeforeUpdate, payload.updates);

if (!result.success) {
Expand All @@ -440,19 +480,7 @@ async function cellUpdate(payload: CellUpdatePayloadType) {

const cell = result.cell as CodeCellType;

if (session.language === 'typescript' && cell.type === 'code' && tsservers.has(session.id)) {
const tsserver = tsservers.get(session.id);

// This isn't intended for renaming, so the filenames
// and their resulting paths are expected to be the same
reopenFileInTsServer(tsserver, session, {
openFilename: cell.filename,
closeFilename: cell.filename,
source: cell.source,
});

requestAllDiagnostics(tsserver, session);
}
refreshCodeCellDiagnostics(session, cell);
}

async function cellRename(payload: CellRenamePayloadType) {
Expand Down Expand Up @@ -725,6 +753,21 @@ async function tsserverQuickInfo(payload: TsServerQuickInfoRequestPayloadType) {
});
}

function refreshCodeCellDiagnostics(session: SessionType, cell: CodeCellType) {
if (session.language === 'typescript' && cell.type === 'code' && tsservers.has(session.id)) {
const tsserver = tsservers.get(session.id);

// This isn't intended for renaming, so the filenames
// and their resulting paths are expected to be the same
reopenFileInTsServer(tsserver, session, {
openFilename: cell.filename,
closeFilename: cell.filename,
source: cell.source,
});

requestAllDiagnostics(tsserver, session);
}
}
wss
.channel('session:*')
.incoming('cell:exec', CellExecPayloadSchema, cellExec)
Expand All @@ -733,6 +776,7 @@ wss
.incoming('cell:update', CellUpdatePayloadSchema, cellUpdate)
.incoming('cell:rename', CellRenamePayloadSchema, cellRename)
.incoming('cell:delete', CellDeletePayloadSchema, cellDelete)
.incoming('cell:format', CellFormatPayloadSchema, cellFormat)
.incoming('ai:generate', AiGenerateCellPayloadSchema, cellGenerate)
.incoming('ai:fix_diagnostics', AiFixDiagnosticsPayloadSchema, cellFixDiagnostics)
.incoming('deps:install', DepsInstallPayloadSchema, depsInstall)
Expand All @@ -747,6 +791,7 @@ wss
)
.outgoing('tsserver:cell:quickinfo:response', TsServerQuickInfoResponsePayloadSchema)
.outgoing('cell:updated', CellUpdatedPayloadSchema)
.outgoing('cell:formatted', CellFormattedPayloadSchema)
.outgoing('cell:error', CellErrorPayloadSchema)
.outgoing('cell:output', CellOutputPayloadSchema)
.outgoing('ai:generated', AiGeneratedCellPayloadSchema)
Expand Down
67 changes: 66 additions & 1 deletion packages/api/session.mts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import {
import { fileExists } from './fs-utils.mjs';
import { validFilename } from '@srcbook/shared';
import { pathToCodeFile } from './srcbook/path.mjs';
import { exec } from 'node:child_process';
import { npmInstall } from './exec.mjs';

const sessions: Record<string, SessionType> = {};

Expand Down Expand Up @@ -296,7 +298,70 @@ export function updateCell(session: SessionType, cell: CellType, updates: CellUp
return updateCodeCell(session, cell, updates);
}
}

async function ensurePrettierInstalled(dir: string): Promise<boolean> {
Copy link
Contributor

@benjreinhart benjreinhart Sep 10, 2024

Choose a reason for hiding this comment

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

Some thoughts here:

  1. 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
  2. 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.
  3. 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?

Copy link
Contributor

@benjreinhart benjreinhart Sep 10, 2024

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

@benjreinhart benjreinhart Sep 10, 2024

Choose a reason for hiding this comment

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

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

const prettierPath = Path.join(dir, 'node_modules', 'prettier');
try {
// check if prettier is installed
await fs.access(prettierPath);
return true;
} catch (error) {
return new Promise<boolean>((resolve) => {
try {
npmInstall({
cwd: dir,
packages: ['prettier'],
Copy link
Contributor

Choose a reason for hiding this comment

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

We should install this in the devDependencies

args: ['--save-dev'],
stdout: () => {},
stderr: (err) => console.error(err),
onExit: (exitCode) => {
if (exitCode === 0) {
resolve(true);
} else {
console.error('Failed to install Prettier:', exitCode);
resolve(false);
}
},
});
} catch (installError) {
console.error('Failed to initiate Prettier installation:', installError);
resolve(false);
}
});
}
}
export async function formatCode(dir: string, fileName: string) {
try {
await ensurePrettierInstalled(dir);

const codeFilePath = pathToCodeFile(dir, fileName);
const command = `npx prettier ${codeFilePath}`;

return new Promise((resolve, reject) => {
exec(command, async (_, stdout, stderr) => {
if (stderr) {
console.error(`exec error: ${stderr}`);
reject(stderr);
return;
}
resolve(stdout);
});
});
} catch (error) {
console.error('Formatting error:', error);
throw error;
}
}
export async function formatAndUpdateCodeCell(session: SessionType, cell: CodeCellType) {
try {
const formattedCode = await formatCode(session.dir, cell.filename);
return updateCodeCell(session, cell, { source: formattedCode } as { source: string });
} catch (error) {
return Promise.resolve({
success: false,
errors: error,
} as UpdateResultType);
}
}
export function sessionToResponse(session: SessionType) {
const result: Pick<SessionType, 'id' | 'cells' | 'language' | 'tsconfig.json' | 'openedAt'> = {
id: session.id,
Expand Down
15 changes: 14 additions & 1 deletion packages/api/srcbook/config.mts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
export function buildJSPackageJson() {
return {
type: 'module',
dependencies: {},
devDependencies: {
prettier: 'latest',
},
prettier: {
semi: true,
singleQuote: true,
},
};
}

Expand All @@ -13,6 +19,13 @@ export function buildTSPackageJson() {
typescript: 'latest',
'@types/node': 'latest',
},
devDependencies: {
prettier: 'latest',
},
prettier: {
semi: true,
singleQuote: true,
},
};
}

Expand Down
9 changes: 9 additions & 0 deletions packages/shared/src/schemas/websockets.mts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ export const CellUpdatePayloadSchema = z.object({
updates: CellUpdateAttrsSchema,
});

export const CellFormatPayloadSchema = z.object({
sessionId: z.string(),
cellId: z.string(),
});

export const AiGenerateCellPayloadSchema = z.object({
sessionId: z.string(),
cellId: z.string(),
Expand Down Expand Up @@ -73,6 +78,10 @@ export const CellUpdatedPayloadSchema = z.object({
cell: CellSchema,
});

export const CellFormattedPayloadSchema = z.object({
cellId: z.string(),
cell: CellSchema,
});
export const AiGeneratedCellPayloadSchema = z.object({
cellId: z.string(),
output: z.string(),
Expand Down
4 changes: 4 additions & 0 deletions packages/shared/src/types/websockets.mts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
CellCreatePayloadSchema,
CellUpdatePayloadSchema,
CellUpdatedPayloadSchema,
CellFormatPayloadSchema,
CellRenamePayloadSchema,
CellDeletePayloadSchema,
AiGenerateCellPayloadSchema,
Expand All @@ -24,12 +25,14 @@ import {
TsServerCellSuggestionsPayloadSchema,
TsServerQuickInfoRequestPayloadSchema,
TsServerQuickInfoResponsePayloadSchema,
CellFormattedPayloadSchema,
} from '../schemas/websockets.mjs';

export type CellExecPayloadType = z.infer<typeof CellExecPayloadSchema>;
export type CellStopPayloadType = z.infer<typeof CellStopPayloadSchema>;
export type CellCreatePayloadType = z.infer<typeof CellCreatePayloadSchema>;
export type CellUpdatePayloadType = z.infer<typeof CellUpdatePayloadSchema>;
export type CellFormatPayloadType = z.infer<typeof CellFormatPayloadSchema>;
export type CellUpdatedPayloadType = z.infer<typeof CellUpdatedPayloadSchema>;
export type CellRenamePayloadType = z.infer<typeof CellRenamePayloadSchema>;
export type CellDeletePayloadType = z.infer<typeof CellDeletePayloadSchema>;
Expand All @@ -43,6 +46,7 @@ export type DepsValidateResponsePayloadType = z.infer<typeof DepsValidateRespons
export type DepsValidatePayloadType = z.infer<typeof DepsValidatePayloadSchema>;

export type CellErrorPayloadType = z.infer<typeof CellErrorPayloadSchema>;
export type CellFormattedPayloadType = z.infer<typeof CellFormattedPayloadSchema>;

export type TsServerStartPayloadType = z.infer<typeof TsServerStartPayloadSchema>;
export type TsServerStopPayloadType = z.infer<typeof TsServerStopPayloadSchema>;
Expand Down
4 changes: 4 additions & 0 deletions packages/web/src/clients/websocket/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import {
AiGenerateCellPayloadSchema,
AiGeneratedCellPayloadSchema,
CellUpdatedPayloadSchema,
CellFormattedPayloadSchema,
CellFormatPayloadSchema,
DepsValidateResponsePayloadSchema,
CellExecPayloadSchema,
CellStopPayloadSchema,
Expand Down Expand Up @@ -35,6 +37,7 @@ const IncomingSessionEvents = {
'cell:output': CellOutputPayloadSchema,
'cell:error': CellErrorPayloadSchema,
'cell:updated': CellUpdatedPayloadSchema,
'cell:formatted': CellFormattedPayloadSchema,
'deps:validate:response': DepsValidateResponsePayloadSchema,
'tsserver:cell:diagnostics': TsServerCellDiagnosticsPayloadSchema,
'tsserver:cell:suggestions': TsServerCellSuggestionsPayloadSchema,
Expand All @@ -50,6 +53,7 @@ const OutgoingSessionEvents = {
'cell:update': CellUpdatePayloadSchema,
'cell:rename': CellRenamePayloadSchema,
'cell:delete': CellDeletePayloadSchema,
'cell:format': CellFormatPayloadSchema,
'ai:generate': AiGenerateCellPayloadSchema,
'ai:fix_diagnostics': AiFixDiagnosticsPayloadSchema,
'deps:install': DepsInstallPayloadSchema,
Expand Down
Loading