Skip to content

fix: vite-dev-server windows #25889

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Feb 22, 2023
Merged

fix: vite-dev-server windows #25889

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
69559fb
chore: update vite in system test
lmiller1990 Nov 7, 2022
ba0fe7d
Merge branch 'develop' of https://github.com/cypress-io/cypress into …
lmiller1990 Nov 30, 2022
ae64ae3
Merge branch 'develop' of https://github.com/cypress-io/cypress into …
lmiller1990 Dec 1, 2022
929958f
Merge branch 'develop' of https://github.com/cypress-io/cypress into …
lmiller1990 Dec 7, 2022
c331d5f
Merge branch 'develop' of https://github.com/cypress-io/cypress into …
lmiller1990 Feb 21, 2023
595c55c
fix: vite-dev-server on windows
lmiller1990 Feb 21, 2023
9321133
fix slash
lmiller1990 Feb 21, 2023
d0cd875
windows
lmiller1990 Feb 21, 2023
d900481
try to run windows again
lmiller1990 Feb 21, 2023
164b6e7
update
lmiller1990 Feb 21, 2023
0057775
chore: comment [skip ci]
lmiller1990 Feb 22, 2023
27ea5fc
update
lmiller1990 Feb 22, 2023
e27734b
Merge branch 'develop' into lmiller/fixing-vite-windows
lmiller1990 Feb 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .circleci/workflows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ mainBuildFilters: &mainBuildFilters
- /^release\/\d+\.\d+\.\d+$/
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- 'update-v8-snapshot-cache-on-develop'
- 'lmiller/fixing-vite-windows'

# usually we don't build Mac app - it takes a long time
# but sometimes we want to really confirm we are doing the right thing
Expand Down Expand Up @@ -68,7 +69,8 @@ windowsWorkflowFilters: &windows-workflow-filters
or:
- equal: [ develop, << pipeline.git.branch >> ]
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- equal: [ 'update-v8-snapshot-cache-on-develop', 'fix-duplicate-and-expired-cookies', << pipeline.git.branch >> ]
- equal: [ 'lmiller/fixing-vite-windows', << pipeline.git.branch >> ]
- equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ]
- matches:
pattern: /^release\/\d+\.\d+\.\d+$/
value: << pipeline.git.branch >>
Expand Down Expand Up @@ -134,7 +136,7 @@ commands:
- run:
name: Check current branch to persist artifacts
command: |
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "astone123/ventura-webpack-permission-testing" ]]; then
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "lmiller/fixing-vite-windows" ]]; then
echo "Not uploading artifacts or posting install comment for this branch."
circleci-agent step halt
fi
Expand Down
8 changes: 7 additions & 1 deletion npm/vite-dev-server/client/initCypressTests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,13 @@ if (supportFile) {

// Using relative path wouldn't allow to load tests outside Vite project root folder
// So we use the "@fs" bit to load the test file using its absolute path
const testFileAbsolutePathRoute = `${devServerPublicPathRoute}/@fs${CypressInstance.spec.absolute}`
let testFileAbsolutePathRoute

if (CypressInstance.config('platform') === 'win32') {
testFileAbsolutePathRoute = `${devServerPublicPathRoute}/@fs/${CypressInstance.spec.absolute}`
} else {
testFileAbsolutePathRoute = `${devServerPublicPathRoute}/@fs${CypressInstance.spec.absolute}`
}
Copy link
Contributor

@marktnoonan marktnoonan Feb 21, 2023

Choose a reason for hiding this comment

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

I was going to suggest a adding a comment here. The difference between the 2 lines is one character in the middle of a string, which is hard to notice at glance.

But we could also change the code so that the difference is more obvious? Something like:

Suggested change
let testFileAbsolutePathRoute
if (CypressInstance.config('platform') === 'win32') {
testFileAbsolutePathRoute = `${devServerPublicPathRoute}/@fs/${CypressInstance.spec.absolute}`
} else {
testFileAbsolutePathRoute = `${devServerPublicPathRoute}/@fs${CypressInstance.spec.absolute}`
}
let testFileAbsolutePathRoute = `${devServerPublicPathRoute}/@fs`
if (CypressInstance.config('platform') === 'win32') {
testFileAbsolutePathRoute += `/${CypressInstance.spec.absolute}`
} else {
testFileAbsolutePathRoute += `${CypressInstance.spec.absolute}`
}

Copy link
Contributor

@mike-plummer mike-plummer Feb 21, 2023

Choose a reason for hiding this comment

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

Might be clearer to remove the OS check and just normalize the absolute path to ensure it doesn't have a leading slash before we work with it

// Normalize path to not include a leading slash (different on Win32 vs Unix)
const normalizedAbsolutePath = CypressInstance.spec.absolute.replace(/^\//, '')
const testFileAbsolutePathRoute = `${devServerPublicPathRoute}/@fs/${normalizedAbsolutePath}`

Copy link
Contributor Author

@lmiller1990 lmiller1990 Feb 21, 2023

Choose a reason for hiding this comment

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

I guess - my concern here is actually the fact we need a comment and a regular expression seems much more complex, or the string concatenation, both seem more complex than just a good old if/else statement, where you can immediately glance and see the difference.

Copy link
Contributor Author

@lmiller1990 lmiller1990 Feb 21, 2023

Choose a reason for hiding this comment

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

I agree we should add a comment here explaining why this is required, let me update that

Copy link
Contributor Author

@lmiller1990 lmiller1990 Feb 22, 2023

Choose a reason for hiding this comment

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

Okay, I thought more - let's go with normalzing, it does look better, after all - thanks!


/* Spec file import logic */
// We need a slash before /src/my-spec.js, this does not happen by default.
Expand Down