-
Notifications
You must be signed in to change notification settings - Fork 143
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
Adding test coverage for vite 6 #2239
Conversation
@@ -321,7 +321,8 @@ function runViteInternalsTest(scenario: Scenario) { | |||
}); | |||
|
|||
Qmodule('vite optimize', function () { | |||
test('vite optimize should succeed', async function (assert) { | |||
// skipped due to https://github.com/vitejs/vite/issues/19316 | |||
QUnit.skip('vite optimize should succeed', async function (assert) { |
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.
This is the only unpleasant surprise so far. vitejs/vite#19316
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.
This has now been fixed upstream and will be released in [email protected] soon, I have updated our CI to target the corresponding beta
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.
correction: the fix didn't get all the way to where we needed to 😞 Keeping an eye on upstream
I see that this PR is actually testing the beta now. not necessary 6, https://pkg.pr.new/vite@main points to beta. you could also use |
The reason this is currently tracking main is because our CI is hitting a bug in Vite@6 vitejs/vite#19316 The first attempt of fixing this was released in beta-1 (that's why we were originally targeting it to see if it would make CI pass) but the real/full fix hasn't been released yet in any beta or release version: vitejs/vite#19356 Personally I don't think we should merge this PR until it's at least in a beta and we can target that as the minimum version in CI (and update it to the release version once it's out) |
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 just flagging this incase anyone feels the urge to merge this too early 😂
tests/scenarios/package.json
Outdated
"execa": "^5.1.1", | ||
"node-fetch": "2.7.0", | ||
"popper.js": "^1.16.1", | ||
"strip-ansi": "^6.0.0", | ||
"testem": "^3.10.1", | ||
"tslib": "^2.6.0", | ||
"typescript": "^5.4.5", | ||
"vite-5": "npm:vite@^5.0.0", | ||
"vite-6": "https://pkg.pr.new/vite@main", |
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.
We should probably wait for a beta to be released rather than keep this floating 🤔
packages/vite/src/build.ts
Outdated
@@ -11,7 +11,7 @@ export function emberBuild(command: string, mode: string, resolvableExtensions: | |||
env['EMBROIDER_RESOLVABLE_EXTENSIONS'] = resolvableExtensions?.join(','); | |||
} | |||
|
|||
if (command === 'build') { | |||
if (command === 'build' || process.env.FORCE_EMBER_CLI_EXIT) { |
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.
We can keep this but let's use something like EMBROIDER_VITE_PREBUILD=false
No description provided.