-
Notifications
You must be signed in to change notification settings - Fork 27k
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
fix: normalize-asset-prefix
adding leading slash when URL assetPrefix
is provided
#68518
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,19 +8,19 @@ function getSocketProtocol(assetPrefix: string): string { | |
protocol = new URL(assetPrefix).protocol | ||
} catch {} | ||
|
||
return protocol === 'http:' ? 'ws' : 'wss' | ||
return protocol === 'http:' ? 'ws:' : 'wss:' | ||
} | ||
|
||
export function getSocketUrl(assetPrefix: string | undefined): string { | ||
const { hostname, port } = window.location | ||
const protocol = getSocketProtocol(assetPrefix || '') | ||
const prefix = normalizedAssetPrefix(assetPrefix) | ||
const protocol = getSocketProtocol(assetPrefix || '') | ||
|
||
// if original assetPrefix is a full URL with protocol | ||
// we just update to use the correct `ws` protocol | ||
if (assetPrefix?.replace(/^\/+/, '').includes('://')) { | ||
return `${protocol}://${prefix}` | ||
if (URL.canParse(prefix)) { | ||
// since normalized asset prefix is ensured to be a URL format, | ||
// we can safely replace the protocol | ||
devjiwonchoi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return prefix.replace(/^http/, 'ws') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could be extra safe and use the |
||
} | ||
|
||
return `${protocol}://${hostname}:${port}${prefix}` | ||
const { hostname, port } = window.location | ||
return `${protocol}//${hostname}${port ? `:${port}` : ''}${prefix}` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { normalizedAssetPrefix } from './normalized-asset-prefix' | ||
|
||
describe('normalizedAssetPrefix', () => { | ||
it('should return an empty string when assetPrefix is nullish', () => { | ||
expect(normalizedAssetPrefix(undefined)).toBe('') | ||
}) | ||
|
||
it('should return an empty string when assetPrefix is an empty string', () => { | ||
expect(normalizedAssetPrefix('')).toBe('') | ||
}) | ||
|
||
it('should return an empty string when assetPrefix is a single slash', () => { | ||
expect(normalizedAssetPrefix('/')).toBe('') | ||
}) | ||
|
||
// we expect an empty string because it could be an unnecessary trailing slash | ||
it('should remove leading slash(es) when assetPrefix has more than one', () => { | ||
expect(normalizedAssetPrefix('///path/to/asset')).toBe('/path/to/asset') | ||
}) | ||
|
||
it('should not remove the leading slash when assetPrefix has only one', () => { | ||
expect(normalizedAssetPrefix('/path/to/asset')).toBe('/path/to/asset') | ||
}) | ||
|
||
it('should add a leading slash when assetPrefix is missing one', () => { | ||
expect(normalizedAssetPrefix('path/to/asset')).toBe('/path/to/asset') | ||
}) | ||
|
||
it('should remove all trailing slash(es) when assetPrefix has one', () => { | ||
expect(normalizedAssetPrefix('/path/to/asset///')).toBe('/path/to/asset') | ||
}) | ||
|
||
it('should return the URL when assetPrefix is a URL', () => { | ||
expect(normalizedAssetPrefix('https://example.com/path/to/asset')).toBe( | ||
'https://example.com/path/to/asset' | ||
) | ||
Comment on lines
+33
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now we return the parsed URL and let |
||
}) | ||
|
||
it('should not leave a trailing slash when assetPrefix is a URL with no pathname', () => { | ||
expect(normalizedAssetPrefix('https://example.com')).toBe( | ||
'https://example.com' | ||
) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,19 @@ | ||
export function normalizedAssetPrefix(assetPrefix: string | undefined): string { | ||
const escapedAssetPrefix = assetPrefix?.replace(/^\/+/, '') || false | ||
// remove all leading slashes and trailing slashes | ||
const escapedAssetPrefix = assetPrefix?.replace(/^\/+|\/+$/g, '') || false | ||
|
||
// assetPrefix as a url | ||
if (escapedAssetPrefix && escapedAssetPrefix.startsWith('://')) { | ||
return escapedAssetPrefix.split('://', 2)[1] | ||
} | ||
|
||
// assetPrefix is set to `undefined` or '/' | ||
// if an assetPrefix was '/', we return empty string | ||
// because it could be an unnecessary trailing slash | ||
if (!escapedAssetPrefix) { | ||
return '' | ||
} | ||
|
||
// assetPrefix is a common path but escaped so let's add one leading slash | ||
if (URL.canParse(escapedAssetPrefix)) { | ||
const url = new URL(escapedAssetPrefix).toString() | ||
return url.endsWith('/') ? url.slice(0, -1) : url | ||
} | ||
|
||
// assuming assetPrefix here is a pathname-style, | ||
// restore the leading slash | ||
return `/${escapedAssetPrefix}` | ||
} |
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.
added
:
for consistency