Skip to content
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

Jump to bottom

fix: injectQuery break relative path #9760

Merged
merged 9 commits into from Aug 24, 2022
Merged

fix: injectQuery break relative path #9760

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fa4d5b8
fix: injectQuery
poyoho Aug 19, 2022
574b035
fix: test
poyoho Aug 19, 2022
fdcf83b
fix: test
poyoho Aug 19, 2022
29fa12a
fix: test
poyoho Aug 19, 2022
e5298bf
fix: pathname
poyoho Aug 20, 2022
58ac423
fix: test
poyoho Aug 20, 2022
bcaea1a
chore: update
poyoho Aug 20, 2022
b1b6cc8
chore: update
poyoho Aug 20, 2022
737c54d
chore: update
poyoho Aug 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 23 additions & 1 deletion packages/vite/src/node/__tests__/utils.spec.ts
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ import {
getPotentialTsSrcPaths,
injectQuery,
isWindows,
normalizePath,
resolveHostname
} from '../utils'

Expand All @@ -14,11 +15,32 @@ describe('injectQuery', () => {
// this test will work incorrectly on unix systems
test('normalize windows path', () => {
expect(injectQuery('C:\\User\\Vite\\Project', 'direct')).toEqual(
'C:/User/Vite/Project?direct'
'C:\\User\\Vite\\Project?direct'
)
})
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure but maybe we need to normalize path.
It seems related to #2435, #2614 but Idk whether this is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It can to exec normalizePath it will reducing '..' and '.' parts. But now it will lose When multiple slashes are found, they're replaced by a single one; when the path contains a trailing slash, it is preserved. On Windows backslashes are used. this feature.

}

test('relative path', () => {
expect(injectQuery('usr/vite/%20a%20', 'direct')).toEqual(
'usr/vite/%20a%20?direct'
)
expect(injectQuery('./usr/vite/%20a%20', 'direct')).toEqual(
'./usr/vite/%20a%20?direct'
)
expect(injectQuery('../usr/vite/%20a%20', 'direct')).toEqual(
'../usr/vite/%20a%20?direct'
)
})
poyoho marked this conversation as resolved.
Show resolved Hide resolved

test('path with protocol', () => {
expect(injectQuery('file:///usr/vite/%20a%20', 'direct')).toMatch(
'file:///usr/vite/%20a%20?direct'
)
expect(injectQuery('http://usr.vite/%20a%20', 'direct')).toMatch(
'http://usr.vite/%20a%20?direct'
)
})

test('path with multiple spaces', () => {
expect(injectQuery('/usr/vite/path with space', 'direct')).toEqual(
'/usr/vite/path with space?direct'
Expand Down
12 changes: 4 additions & 8 deletions packages/vite/src/node/utils.ts
View file
Open in desktop
Expand Up @@ -311,14 +311,10 @@ export function injectQuery(url: string, queryToInject: string): string {
if (resolvedUrl.protocol !== 'relative:') {
resolvedUrl = pathToFileURL(url)
}
let { protocol, pathname, search, hash } = resolvedUrl
if (protocol === 'file:') {
pathname = pathname.slice(1)
}
pathname = decodeURIComponent(pathname)
return `${pathname}?${queryToInject}${search ? `&` + search.slice(1) : ''}${
hash ?? ''
}`
const { search, hash } = resolvedUrl
return `${url.split('?')[0]}?${queryToInject}${
search ? `&` + search.slice(1) : ''
}${hash ?? ''}`
}

const timestampRE = /\bt=\d{13}&?\b/
Expand Down