Move Teleport Connect over to Vite by ryanclark · Pull Request #36921 · gravitational/teleport

ryanclark · 2024-01-19T12:32:32Z

This moves Connect's build & development tooling over to Vite.

In order to use Vite (which doesn't like CommonJS includes unless it's a 3rd party module), I had to swap over our JS protobuf generation to TypeScript - this is actually a nice advantage, because we can now use the protobuf types in the Teleport codebase. It also fixes a few type issues/FIXMEs in the codebase. I moved user preferences over to using the protobuf files as there was intertwined logic between Teleterm, Teleport & the gRPC representation, so I simplified it.

Sorry for the large PR. These changes were a result of changing one thing and then fixing the errors, changing another, fixing those, etc - it was hard to keep these concise and in separate, ordered commits.

Speed improvements

Start -
Vite - 5.89 seconds
Webpack 22.7 seconds

Build -
Vite - 14.28 seconds
Vite (bytecode) - 168.26 seconds
Webpack - 92.64 seconds

Caveats

The oneOf representation in @protobuf-ts/plugin only works if strictNullChecks is enabled. I tried to enable this, but I couldn't get Connect to only be checked and not Teleport.

To get around this, I added some helpers to ensure type safety. There's only 3 different oneOf values to check in the codebase, and I don't think it's too tedious to add new helpers for new values. If the protobuf representation changes, the types will break, so they will always be in sync.

Things to test

public-teleport-github-review-bot · 2024-01-19T12:33:10Z

@ryanclark - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

ryanclark · 2024-01-19T12:51:02Z

Note - I had tried to add strictNullChecks, so there may be code changes to account for that, but they're not needed now. Please point out any you see and I'll revert

ravicious

I went through the tshd client. I'm yet to build the app, but I definitely want to look into how the final packaged app looks like structure-wise. I can also do a local signed build before we take it to CI.

orca-security-us

Orca Security Scan Summary

Status	Check	Issues by priority
Passed	Infrastructure as Code	0 0 0 0	View in Orca
Failed	Vulnerabilities	0 1 0 0	View in Orca
Passed	Secrets	0 0 0 0	View in Orca

ravicious · 2024-01-22T15:20:39Z

Grzegorz and I want to go through Connect's test plan again once we merge this into master and make a dev build off of v15 with these changes. But what about the changes in Web UI, how are we going to test them?

ravicious · 2024-01-22T15:43:43Z

If we merge it to master and backport to v15 only after a few days, it'll be inconvenient to anyone working on the Web UI code that interacts with types from protos. Once we get everything working in this PR, we should probably do tests on the v15 dev build and only then merge to master, followed by a quick backport to v15.

ravicious · 2024-01-23T11:35:29Z

Could someone double check this change here?

ravicious · 2024-01-23T12:04:42Z

This change should be fine I think, @gzdunek AFAIK it was to accommodate for some weird Timestamp shenanigans in protos?

I think so, I will make sure that the correct date is sent with the usage events.

Except for the wrong environment, usage events are sent correctly.

ravicious · 2024-01-23T12:19:47Z

This is so much nicer than how I implemented it!

ravicious

I went through the whole PR and everything appears to be okay just from looking at the code alone. We'll run the Connect test plan on 15.0.0-dev.ravicious.10 just to double check.

ravicious

Okay, we ran the test plan against v15.0.0-dev.ravicious.10 and it looks everything works fine. What's next, do we wait for the protobuf PRs to land in master?

ryanclark · 2024-01-24T13:29:02Z

Okay, we ran the test plan against v15.0.0-dev.ravicious.10 and it looks everything works fine. What's next, do we wait for the protobuf PRs to land in master?

Yep

Otherwise process.env in the main process is always replaced with an object containing just NODE_ENV. We need process.env to be properly populated since it's then used when spawning tsh daemon and other processes. Using define like this guarantees that only callsites using process.env.NODE_ENV directly will be affected.

…C UserPreferences object

…orrectly

…rface

ryanclark added the no-changelog Indicates that a PR does not require a changelog entry label Jan 19, 2024

github-actions Bot added size/xl ui labels Jan 19, 2024

github-actions Bot requested review from avatus and gzdunek January 19, 2024 12:33

gzdunek requested review from ravicious and removed request for avatus January 19, 2024 12:45

ravicious reviewed Jan 19, 2024

View reviewed changes

Comment thread web/packages/teleterm/src/services/tshd/createClient.ts

ravicious reviewed Jan 19, 2024

View reviewed changes

ravicious self-requested a review January 19, 2024 15:10

orca-security-us Bot reviewed Jan 22, 2024

View reviewed changes

ryanclark mentioned this pull request Jan 22, 2024

Generate TypeScript protobuf files #37009

Merged

ryanclark force-pushed the ryan/connect-vite branch from cc118a6 to 2a6b4a2 Compare January 22, 2024 12:17

gzdunek reviewed Jan 22, 2024

View reviewed changes

Comment thread web/packages/teleterm/electron.vite.config.mts Outdated

ravicious reviewed Jan 22, 2024

View reviewed changes

Comment thread package.json Outdated

gzdunek reviewed Jan 22, 2024

View reviewed changes

Comment thread web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts Outdated

Comment thread web/packages/teleterm/src/helpers.ts Outdated

ravicious reviewed Jan 22, 2024

View reviewed changes

Comment thread web/packages/teleterm/package.json Outdated

ravicious reviewed Jan 23, 2024

View reviewed changes

Comment thread .eslintignore Outdated

ravicious reviewed Jan 23, 2024

View reviewed changes

Comment thread web/packages/teleport/src/User/UserContext.tsx Outdated

Copy link
Copy Markdown

Member

ravicious Jan 23, 2024

Choose a reason for hiding this comment

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

Could someone double check this change here?

ravicious reviewed Jan 23, 2024

View reviewed changes

Comment thread web/packages/teleterm/src/types.ts Outdated

Copy link
Copy Markdown

Member

ravicious Jan 23, 2024

Choose a reason for hiding this comment

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

This is so much nicer than how I implemented it!

ravicious reviewed Jan 23, 2024

View reviewed changes

ravicious approved these changes Jan 23, 2024

View reviewed changes

gzdunek approved these changes Jan 23, 2024

View reviewed changes

gzdunek reviewed Jan 25, 2024

View reviewed changes

Comment thread web/packages/teleport/src/services/userPreferences/userPreferences.ts Outdated

ravicious and others added 8 commits January 26, 2024 12:09

Define NODE_ENV for main and renderer

566124b

Add support for the cluster preferences backend mismatch from the gRP…

eff0b87

…C UserPreferences object

Return resourceIds from preferences, convert preferences before updating

4de3e59

Future proof all of the clusterPreferences object getting converted c…

d4d802a

…orrectly

Fix cluster preferences for leaf clusters

6fb264f

Use correct AssistViewMode enum, remove the old one and unused inte…

07f3806

…rface

Lint fix

70adc76

ryanclark force-pushed the ryan/connect-vite branch 2 times, most recently from a5410b4 to e9df594 Compare January 26, 2024 11:13

Add back in JS protobuf stuff, it can be removed in a later PR

93d66cb

ryanclark force-pushed the ryan/connect-vite branch from e9df594 to 93d66cb Compare January 26, 2024 11:15

ryanclark added 2 commits January 26, 2024 12:23

Remove List from property

f1d3496

Add license header

401ff0d

ryanclark mentioned this pull request Jan 26, 2024

Remove Webpack #37332

Closed

gzdunek approved these changes Jan 26, 2024

View reviewed changes

Comment thread web/packages/teleterm/src/ui/AppInitializer/AppInitializer.tsx Outdated

Remove console.logs

b8577f9

ryanclark enabled auto-merge January 26, 2024 14:46

ryanclark added this pull request to the merge queue Jan 26, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 26, 2024

gzdunek added this pull request to the merge queue Jan 26, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 26, 2024

gzdunek added this pull request to the merge queue Jan 26, 2024

Merged via the queue into master with commit 200553c Jan 26, 2024

gzdunek deleted the ryan/connect-vite branch January 26, 2024 16:00

ravicious mentioned this pull request Jan 29, 2024

Fix gRPC logging interceptor in Connect #37410

Merged

ryanclark mentioned this pull request Jan 30, 2024

[v15] Move Teleport Connect over to Vite #37512

Merged

This was referenced Feb 27, 2024

Update grpc_tools_node_protoc_ts to 5.3.2 #38652

Closed

Clean up after grpc_tools_node_protoc_ts removal #38725

Open

Conversation

ryanclark commented Jan 19, 2024 • edited by ravicious Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Speed improvements

Caveats

Things to test

Uh oh!

public-teleport-github-review-bot Bot commented Jan 19, 2024

Uh oh!

ryanclark commented Jan 19, 2024

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orca-security-us Bot left a comment

Choose a reason for hiding this comment

Orca Security Scan Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravicious commented Jan 22, 2024

Uh oh!

ravicious commented Jan 22, 2024

Uh oh!

Uh oh!

Uh oh!

ravicious Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

ravicious Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

gzdunek Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

gzdunek Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

ravicious Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

ryanclark commented Jan 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ryanclark commented Jan 19, 2024 •

edited by ravicious

Loading