Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a463a1b40
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
pnpm start-term fails with:
The electron vite main config output format must be "cjs" or "es".
I guess it's because alex8088/electron-vite#894?
|
Fixed these |
b6db2ac to
8fae01b
Compare
this was added because crypto.randomUUID in the browser is only available in an HTTPS/secure context. i don't remember the usecase for it, but that was the reason for the polyfill do you remember @rudream |
|
disregard comment about the polyfill, i must be misremembering. it was only added for browser support #26368 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9935bcc427
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
zmb3
left a comment
There was a problem hiding this comment.
LGTM so long as someone from the Connect team approves
gzdunek
left a comment
There was a problem hiding this comment.
Connect on macOS works fine.
| external: [ | ||
| 'electron', | ||
| /^electron\/.+/, | ||
| ...builtinModules.flatMap(m => [m, `node:${m}`]), | ||
| ...deps, | ||
| new RegExp(`^(${deps.join('|')})/.+`), | ||
| ], |
There was a problem hiding this comment.
From my testing, we can only externalize ['electron', 'node-pty'] and the app still works. However, it slightly increases the final DMG size (159286403 vs 158478273), so it seems reasonable to keep the current setup.
There was a problem hiding this comment.
Thanks for taking the time to test
* Upgrade Vite to v8 * Remove crypto.randomUUID polyfill * Remove tsconfigPathsPlugin from everywhere * pnpm format * Update electron vite config with new options * Fix the loading of ace editor * rollupOptions -> rolldownOptions in gen-event-reference-config * Import makeEvent directly to avoid including frontend code * Fix electron config * pnpm format * Ensure ace extensions load after ace is loaded * pnpm format * Change external dependencies to only apply to main/preload * Remove unused eslint directive
* Upgrade Vite to v8 * Remove crypto.randomUUID polyfill * Remove tsconfigPathsPlugin from everywhere * pnpm format * Update electron vite config with new options * Fix the loading of ace editor * rollupOptions -> rolldownOptions in gen-event-reference-config * Import makeEvent directly to avoid including frontend code * Fix electron config * pnpm format * Ensure ace extensions load after ace is loaded * pnpm format * Change external dependencies to only apply to main/preload * Remove unused eslint directive
* Upgrade Vite to v8 (#64600) * Upgrade Vite to v8 * Remove crypto.randomUUID polyfill * Remove tsconfigPathsPlugin from everywhere * pnpm format * Update electron vite config with new options * Fix the loading of ace editor * rollupOptions -> rolldownOptions in gen-event-reference-config * Import makeEvent directly to avoid including frontend code * Fix electron config * pnpm format * Ensure ace extensions load after ace is loaded * pnpm format * Change external dependencies to only apply to main/preload * Remove unused eslint directive * Add pnpm-lock * Update @swc/core and @swc/plugin-styled-components to match master * pnpm format * Remove `rolldown` dependency (#65330) (cherry picked from commit fc19088) * Remove HTTP2 headers * Upgrade vite to 8.0.7 --------- Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com>
* Upgrade Vite to v8 * Remove crypto.randomUUID polyfill * Remove tsconfigPathsPlugin from everywhere * pnpm format * Update electron vite config with new options * Fix the loading of ace editor * rollupOptions -> rolldownOptions in gen-event-reference-config * Import makeEvent directly to avoid including frontend code * Fix electron config * pnpm format * Ensure ace extensions load after ace is loaded * pnpm format * Change external dependencies to only apply to main/preload * Remove unused eslint directive
This upgrades Vite to v8.
The build time of
egoes from ~14s to ~7s, a 2x speed increase.Rolldown (the new Rust version of rollup) was complaining about us including
crypto, which was included by thecrypto.randomUUID()polyfill. This method is supported in all major browsers, so I removed it.Vite 8 now supports following paths in
tsconfig.jsonitself, so we can remove thevite-tsconfig-pathsplugin.Will manually backport to v18 as I want to sync all the tooling dependencies first.
Manual Test Plan
Test Environment
Deployed to https://ryantest.cloud.gravitational.io
Test Cases
pnpm start-teleport-e) loads correctly and works as expected