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

Adds mechanism to support managed npm install runs from within the app builder #381

Merged
merged 7 commits into from
Oct 18, 2024

Conversation

1egoman
Copy link
Collaborator

@1egoman 1egoman commented Oct 17, 2024

This change adds a new context which can be used from within the app builder experience to programatically run npm install or npm install <packagename>. This builds on top of much of the existing package.json management logic originally built for srcbook notebooks. Also note that logs stream in while the npm install occurs live!

In addition, I've introduced a very barebones interface that can be used to try it out. Note that a more thought through interface is definitely the long term intention - I just wanted to get something out to get folks moving, and plan to follow up with something later here.

Finally, I've made some small tweaks to the statusbar panel to fix some rendering weirdnesses I encountered.

High level walkthrough

Screenshot 2024-10-17 at 3 56 26 PM
Untitled.mov
Screenshot 2024-10-17 at 3 57 44 PM Screenshot 2024-10-17 at 3 58 25 PM

Comment on lines 151 to 171
const process = npmInstall({
args: [],
cwd: pathToApp(app.externalId),
packages: payload.packages ?? undefined,
stdout: (data) => {
conn.reply(`app:${app.externalId}`, 'dependencies:install:log', {
log: { type: 'stdout', data: data.toString('utf8') },
});
},
stderr: (data) => {
conn.reply(`app:${app.externalId}`, 'dependencies:install:log', {
log: { type: 'stderr', data: data.toString('utf8') },
});
},
onExit: (code) => {
conn.reply(`app:${app.externalId}`, 'dependencies:install:status', {
status: code === 0 ? 'complete' : 'failed',
code,
});
},
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's where the npm install command is executed in the api service.

Comment on lines +5 to +36
const { status, output, npmInstall } = usePackageJson();

return (
<div className="flex flex-col gap-4 px-5 w-[360px]">
<div>
<Button onClick={() => npmInstall()} disabled={status === 'installing'}>
Run npm install
</Button>
</div>
<div>
<Button
onClick={() => npmInstall(['uuid'])}
variant="secondary"
disabled={status === 'installing'}
>
Run npm install uuid
</Button>
</div>

{status !== 'idle' ? (
<>
<span>
Status: <code>{status}</code>
</span>
<pre className="font-mono text-sm bg-tertiary p-2 overflow-auto rounded-md border">
{/* FIXME: disambiguate between stdout and stderr in here using n.type! */}
{output.map((n) => n.data).join('\n')}
</pre>
</>
) : null}
</div>
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again just wanted to call out here that this is an interim interface! Clicking the two buttons should allow folks to test both the npm install and npm install <packagename> paths.

Comment on lines +12 to +18
export interface PackageJsonContextValue {
npmInstall: (packages?: string[]) => void;
status: NpmInstallStatus;
installing: boolean;
failed: boolean;
output: Array<OutputType>;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A lot of this interface should look familiar to the similar context that exists for srcbook notebooks - I originally tried to go down the path of making that reusable, but that proved to be too challenging to do quickly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. We can take both later and extract an optimal interface, but this is not urgent at all. Good call

<button onClick={() => addError({ type: 'vite_error', contents: 'Bogus error' })}>
Trigger
</button>
</div>
<iframe className="w-full h-full" src={url} title="App preview" />
</div>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😬 I forgot I left this in when I merged my last change... sorry!

Copy link
Contributor

Choose a reason for hiding this comment

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

We're moving fast and I love it. Tiny price to pay

@1egoman 1egoman marked this pull request as ready for review October 17, 2024 20:12
@1egoman
Copy link
Collaborator Author

1egoman commented Oct 17, 2024

Also worth calling out: there's nothing right now serverside to prevent two npm installs from happening concurrently. I can probably add some more client side validation to make that case impossible at least within the context of the client but I don't know how much of a real problem this actually is in practice.

Copy link
Contributor

@nichochar nichochar left a comment

Choose a reason for hiding this comment

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

LGTM. Some minor nits on naming mostly, but going to merge and fix them myself. Thanks @1egoman

@@ -150,6 +186,7 @@ export function register(wss: WebSocketServer) {
.channel('app:<appId>')
.on('preview:start', PreviewStartPayloadSchema, previewStart)
.on('preview:stop', PreviewStopPayloadSchema, previewStop)
.on('dependencies:install', DependenciesInstallPayloadSchema, dependenciesInstall)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: in the notebook we use deps:install. I prefer normalizing.

Comment on lines +12 to +18
export interface PackageJsonContextValue {
npmInstall: (packages?: string[]) => void;
status: NpmInstallStatus;
installing: boolean;
failed: boolean;
output: Array<OutputType>;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. We can take both later and extract an optimal interface, but this is not urgent at all. Good call

@nichochar nichochar merged commit 3948ca0 into main Oct 18, 2024
3 checks passed
@nichochar nichochar deleted the rgaus/add-npm-install branch October 18, 2024 00:25
Copy link
Contributor

@nichochar nichochar left a comment

Choose a reason for hiding this comment

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

Followed up here. Only naming. Feature works well!

@@ -179,3 +179,19 @@ export const PreviewStatusPayloadSchema = z.union([

export const PreviewStartPayloadSchema = z.object({});
export const PreviewStopPayloadSchema = z.object({});

export const DependenciesInstallPayloadSchema = z.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awefully close to DepsInstallPayloadSchema:

export const DepsInstallPayloadSchema = z.object({
    packages: z.array(z.string()).optional(),
});

might merge them

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants