-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
cmd/tsconnect: switch UI to Preact #5300
Conversation
Reduces the amount of boilerplate to render the UI and makes it easier to respond to state changes (e.g. machine getting authorized, netmap changing, etc.) Preact adds ~13K to our bundle size (5K after Brotli) thus is a neglibible size contribution. We mitigate the delay in rendering the UI by having a static placeholder in the HTML. Required bumping the esbuild version to pick up evanw/esbuild#2349, which makes it easier to support Preact's JSX code generation. Fixes #5137 Fixes #5273 Signed-off-by: Mihai Parparita <mihai@tailscale.com>
Tagging @phirework for some eyes on this! |
return ( | ||
<div | ||
class="flex-grow bg-black p-2 overflow-hidden" | ||
ref={(node) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like the function is returning anything, what is the ref handler holding onto?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using ref
as a callback to know when the component has been rendered and there's a DOM node I can attach the xterm.js instance to. If there's a more idiomatic way with to do with hooks I'm happy to switch (we never did a full hooks port at my old job, so I don't have much production experience with them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh that's clever. The more idiomatic way would be to combine a useRef
on this div with a useEffect
that has that ref in the dependency array, like what we're doing with the ACL editor but I think the logic is the same in the end. Up to you if you want to switch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping this for now mostly so I can merge the commit as is (don't have a dev environment on my vacation machine, but I don't want to have this PR block Charlotte's work by having that be done on top of the old DOM code). Will look into switching to hooks next week.
return ( | ||
<div | ||
class="flex-grow bg-black p-2 overflow-hidden" | ||
ref={(node) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh that's clever. The more idiomatic way would be to combine a useRef
on this div with a useEffect
that has that ref in the dependency array, like what we're doing with the ACL editor but I think the logic is the same in the end. Up to you if you want to switch!
Reduces the amount of boilerplate to render the UI and makes it easier to
respond to state changes (e.g. machine getting authorized, netmap changing,
etc.)
Preact adds ~13K to our bundle size (5K after Brotli) thus is a neglibible
size contribution. We mitigate the delay in rendering the UI by having a static
placeholder in the HTML.
Required bumping the esbuild version to pick up evanw/esbuild#2349, which
makes it easier to support Preact's JSX code generation.
Fixes #5137
Fixes #5273
Signed-off-by: Mihai Parparita mihai@tailscale.com