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(dashboard, ui): Support TypeScript in the playground #919

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

akitaSummer
Copy link
Contributor

@changeset-bot
Copy link

changeset-bot bot commented Jun 2, 2023

🦋 Changeset detected

Latest commit: 10297c9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@lagon/dashboard Patch
@lagon/ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jun 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
storybook ❌ Failed (Inspect) Aug 6, 2023 4:47pm

@vercel
Copy link

vercel bot commented Jun 2, 2023

@akitaSummer is attempting to deploy a commit to the Lagon Team on Vercel.

A member of the Team first needs to authorize it.

@QuiiBz
Copy link
Member

QuiiBz commented Jun 4, 2023

Thanks for this PR, I'll work on #887 first (will take a bit of time, it requires a lot of work)

@QuiiBz
Copy link
Member

QuiiBz commented Jun 4, 2023

Side note: I'm not sure if this is the right approach (we might need more changes):

  • I can write a Function in TS in the playground
  • Let's deploy it, cool it works
  • Now back to the playground, and the Function's code doesn't have the types anymore

This is "expected" since this approach bundles the code before uploading it, but that means we lose the original TS code. We might want to also save the original TS code and upload it alongside the bundled code, to be able to use it later (but only for deployments in the playground)

@akitaSummer
Copy link
Contributor Author

Side note: I'm not sure if this is the right approach (we might need more changes):

  • I can write a Function in TS in the playground
  • Let's deploy it, cool it works
  • Now back to the playground, and the Function's code doesn't have the types anymore

This is "expected" since this approach bundles the code before uploading it, but that means we lose the original TS code. We might want to also save the original TS code and upload it alongside the bundled code, to be able to use it later (but only for deployments in the playground)

@QuiiBz
I understand your point. I used wasm because the resources on our server are very valuable, and I don't want esbuild to consume resources on our server. My suggestion is that if you want to support typescript code echoing, you might need to change the current table structure and save the original ts files to S3. I understand that the playground is only for an experience, and for more complex development, it's best to do it locally. If the playground supports typescript code echoing, it's better to also support code uploaded by the CLI. This would make the logic complex and I'm not sure if it's the right thing to do.

@akitaSummer
Copy link
Contributor Author

@QuiiBz
The TS files shouldn't get lost now. You can give it a try.

Copy link
Member

@QuiiBz QuiiBz left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! Some feedback:

packages/dashboard/lib/hooks/useEsbuild.ts Outdated Show resolved Hide resolved
packages/dashboard/pages/index.tsx Outdated Show resolved Hide resolved
packages/dashboard/lib/hooks/useEsbuild.ts Show resolved Hide resolved
packages/dashboard/lib/hooks/useEsbuild.ts Show resolved Hide resolved
}),
...(tsCode.length > 0
? [
fetch(deployment.tsCodeUrl!, {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it makes sense to only upload the TS code when we previously had a TS code? Meaning TS is only supported when creating Functions via the dashboard, and not when creating them via the CLI (which makes sense because we can't support this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand what you're saying. Firstly, V8 can only run JS code, which means we only have two options: one is to store only TS code and transpile it to JS at runtime, but it will reduce our performance, so I don't recommend this approach. The other option is to store both TS and JS code, but it will cost us more space. I chose the latter because for users, response time is the most important factor. Also, as you mentioned, the CLI results will only support JS, and I think this is unavoidable in the current scenario. If you want to avoid this problem, the playground would have to implement a file system (such as using a webcontainer), which would involve a lot of work and is not within the scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Completely agree. We could probably add a new column to the Function table that will be used to check if the function has been created using the CLI or the Dashboard. With that, we could easily only show the Playground button if the Function has been created from the Dashboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Give it a try and see if it meets your requirements.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I'll take a look this weekend and we then should be ready to merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QuiiBz How is the progress, can this pr be merged?

Copy link
Member

@QuiiBz QuiiBz left a comment

Choose a reason for hiding this comment

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

I was really busy recently, apologize for the delay

@@ -143,6 +143,7 @@ model Deployment {
triggerer String @default("Lagon")
commit String?
isProduction Boolean @default(false)
platform String @default("CLI")
Copy link
Member

Choose a reason for hiding this comment

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

The platform should be set to a Function and not a Deployment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Give it a try and see if it meets your requirements.

@@ -38,15 +39,17 @@ export const LogLine = ({ date, level = 'info', message }: LogLineProps) => {
return (
<div className={cx(['group flex w-full justify-between rounded-md px-2 py-1', containerStyle])}>
<div className="flex items-start gap-4">
<p className={cx(['w-36 whitespace-pre text-sm', dateStyle])}>{date.toLocaleString('en-US')}</p>
{date && <p className={cx(['w-36 whitespace-pre text-sm', dateStyle])}>{date.toLocaleString('en-US')}</p>}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{date && <p className={cx(['w-36 whitespace-pre text-sm', dateStyle])}>{date.toLocaleString('en-US')}</p>}
{date ? <p className={cx(['w-36 whitespace-pre text-sm', dateStyle])}>{date.toLocaleString('en-US')}</p> : null}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

>
Copy
</span>
{!hiddenCopy && (
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a descriptive name instead of double negation:

Suggested change
{!hiddenCopy && (
{showCopy ? (

Copy link
Contributor Author

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