-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[build] Chore: Upgrade rollup and vite to latest versions #6018
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Thank you for doing this! |
c072394
to
3f0f885
Compare
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.
great work thankyou for this upgrade!
Thank you for doing this Bob! I think that's reasonable, the point of the prod build since we first introduced in the CI has always been to have some degree of certainty on a build that is slightly different than dev. We had some issues in the past with the rollup bundling that we didn't catch until it broke in prod. Currently, I think that the CI step that runs all E2E on prod already gives us this confidence so we can move on with your proposal. |
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 noticed that the port is 5173 now, I wonder how hard it is to make port 3000 back, no strong opinions though. Merging now to avoid the rebase churn.
@@ -1,45 +0,0 @@ | |||
#!/usr/bin/env 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.
I assume this is an unrelated cleanup
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.
Yes, this was supposed to be deleted in #5876
@@ -207,7 +207,7 @@ describe('LexicalNodeHelpers tests', () => { | |||
test('DFS of empty ParagraphNode returns only itself', async () => { | |||
const editor: LexicalEditor = testEnv.editor; | |||
|
|||
let paragraphKey; | |||
let paragraphKey: string; |
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.
let paragraphKey: string; | |
let paragraphKey: NodeKey; |
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.
honestly I only did it this way because there's a plethora of other tests that use string for keys, might be a good lint for us to find variables named /[Kk]ey$/
with a string type!
@@ -26,11 +36,9 @@ export const LEGACY_EVENTS = process.env.E2E_EVENTS_MODE === 'legacy-events'; | |||
export const SAMPLE_IMAGE_URL = | |||
E2E_PORT === 3000 | |||
? '/src/images/yellow-flower.jpg' | |||
: '/assets/yellow-flower.a2a7c7a2.jpg'; | |||
: findAsset('yellow-flower*.jpg'); |
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.
So much better, finding the suffix was always time-consuming
…ite upgrade regression)
Description
rollup and vite have not been upgraded in some time, it makes sense to keep them up to date
Packages upgraded:
I also upgraded most of the workflow actions because I was getting a spurious cache failure. I did not update to actions/upload-artifact@v4 which seemed like it might need some other config changes https://github.com/actions/upload-artifact?tab=readme-ov-file#breaking-changes
Other changes:
setupEnv.ts
was getting tree shaken in prod because it was only performing side-effects so I refactored it and the settings panel which didn't really work correctly previouslyCloses: #6012
Test plan
Before
Builds work and pass all tests
After
Builds still work and still pass all the tests