Skip to content

Commit 98badca

Browse files
vzaidmanfacebook-github-bot
authored andcommitted
remove support for relative urls when parsing bundle options from url
Summary: Changelog: [Internal] Remove support for relative urls when parsing bundle options from url. This seems to have not been used. Working on `parseBundleOptionsFromBundleRequestUrl`, I noticed we handle a use-case that is not clear why would we support. It complicates the logic of that very complicated function without a sufficiently good reason, besides being a left-over from an older HMR logic. While we introduce some semi-breaking-changes to how urls are handled, it's a good opportunity to make the function stricter in terms of what it expects to receive as an input. Reviewed By: robhogan Differential Revision: D79809398 fbshipit-source-id: 3b73e189ef163d1a965a71883c83e826b8418f0f
1 parent 71e158a commit 98badca

5 files changed

Lines changed: 31 additions & 49 deletions

File tree

packages/metro/src/HmrServer.js

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,6 @@ type ClientGroup = {
4949
+graphOptions: GraphOptions,
5050
};
5151

52-
// This is a bit weird but this is the recommended way of getting around "URL" demanding to have a valid protocol
53-
// for when handling relative URLs: https://nodejs.org/docs/latest-v24.x/api/url.html#urlresolvefrom-to
54-
const RESOLVE_BASE_URL = 'resolve://';
55-
5652
function send(sendFns: Array<(string) => void>, message: HmrMessage): void {
5753
const strMessage = JSON.stringify(message);
5854
sendFns.forEach((sendFn: string => void) => sendFn(strMessage));
@@ -157,7 +153,7 @@ export default class HmrServer<TClient: Client> {
157153
if (clientGroup != null) {
158154
clientGroup.clients.add(client);
159155
} else {
160-
const clientUrl = new URL(requestUrl, RESOLVE_BASE_URL);
156+
const clientUrl = new URL(requestUrl);
161157

162158
// Prepare the clientUrl to be used as sourceUrl in HMR updates.
163159
clientUrl.protocol = 'http';

packages/metro/src/Server.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -622,7 +622,7 @@ export default class Server {
622622
const pathname = urlObj.pathname || '';
623623

624624
// using this Metro particular convention for decoding URL paths into file paths
625-
const decodedPathname = pathname
625+
const filePathname = pathname
626626
.split('/')
627627
.map(segment => decodeURIComponent(segment))
628628
.join('/');
@@ -639,7 +639,7 @@ export default class Server {
639639
});
640640

641641
if (this._serverOptions && this._serverOptions.onBundleBuilt) {
642-
this._serverOptions.onBundleBuilt(decodedPathname);
642+
this._serverOptions.onBundleBuilt(filePathname);
643643
}
644644
} else if (pathname.endsWith('.map')) {
645645
// Chrome dev tools may need to access the source maps.
@@ -671,8 +671,8 @@ export default class Server {
671671
let handled = false;
672672
for (const [pathnamePrefix, normalizedRootDir] of this
673673
._sourceRequestRoutingMap) {
674-
if (decodedPathname.startsWith(pathnamePrefix)) {
675-
const relativeFilePathname = decodedPathname.substr(
674+
if (filePathname.startsWith(pathnamePrefix)) {
675+
const relativeFilePathname = filePathname.substr(
676676
pathnamePrefix.length,
677677
);
678678
await this._processSourceRequest(

packages/metro/src/__tests__/HmrServer-test.js

Lines changed: 6 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -227,31 +227,12 @@ describe('HmrServer', () => {
227227
});
228228

229229
test('should retrieve for relative urls without host and protocol', async () => {
230-
await connect('/hot?bundleEntry=EntryPoint.js&platform=ios', undefined, {
231-
baseUrl: '',
232-
});
233-
234-
expect(getRevisionByGraphIdMock).toBeCalledWith(
235-
getGraphId(
236-
'/root/EntryPoint.js',
237-
{
238-
customTransformOptions: {},
239-
dev: true,
240-
hot: true,
241-
minify: false,
242-
platform: 'ios',
243-
type: 'module',
244-
unstable_transformProfile: 'default',
245-
},
246-
{
247-
shallow: false,
248-
lazy: false,
249-
unstable_allowRequireContext: false,
250-
resolverOptions: {
251-
dev: true,
252-
},
253-
},
254-
),
230+
expect(
231+
connect('/hot?bundleEntry=EntryPoint.js&platform=ios', undefined, {
232+
baseUrl: '',
233+
}),
234+
).rejects.toThrowError(
235+
'Expecting the request url to have a valid protocol, e.g. "http://", "https://", or "//"',
255236
);
256237
});
257238

packages/metro/src/lib/__tests__/parseBundleOptionsFromBundleRequestUrl-test.js

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,16 @@ describe('parseBundleOptionsFromBundleRequestUrl', () => {
3939
).toMatchObject({platform: 'test'});
4040
});
4141

42+
test('relative urls fail to be parsed', () => {
43+
expect(() =>
44+
parseBundleOptionsFromBundleRequestUrl('my/bundle.bundle', new Set([])),
45+
).toThrowError(
46+
'Expecting the request url to have a valid protocol, e.g. "http://", "https://", or "//"',
47+
);
48+
});
49+
4250
test.each(['absolute', 'relative'])(
43-
'%s urls- infers the source url and source map url from the pathname',
51+
'%s protocol urls- infers the source url and source map url from the pathname',
4452
type => {
4553
const protocol = type === 'absolute' ? 'http:' : '';
4654
expect(
@@ -75,14 +83,6 @@ describe('parseBundleOptionsFromBundleRequestUrl', () => {
7583
});
7684
});
7785

78-
test('retrieves stuff from HMR urls', () => {
79-
expect(
80-
parseBundleOptionsFromBundleRequestUrl('my/bundle.bundle', new Set([])),
81-
).toMatchObject({
82-
entryFile: './my/bundle',
83-
});
84-
});
85-
8686
test.each(['absolute', 'relative'])(
8787
'%s urls with ascii characters are encoded correctly',
8888
type => {

packages/metro/src/lib/parseBundleOptionsFromBundleRequestUrl.js

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,16 +70,21 @@ export default function parseBundleOptionsFromBundleRequestUrl(
7070
hash,
7171
} = new URL(rawNonJscSafeUrlEncodedUrl, RESOLVE_BASE_URL /* baseURL */);
7272

73+
// e.g. "//localhost:8081/foo/bar.js?platform=ios"
7374
const isRelativeProtocol = rawNonJscSafeUrlEncodedUrl.startsWith('//');
75+
76+
// e.g. "/foo/bar.js?platform=ios"
7477
const isNoProtocol =
7578
!isRelativeProtocol && _tempProtocol + '//' === RESOLVE_BASE_URL;
7679

77-
// TODO: next diff (D79809398) will remove the support for "isNoProtocol" to make the requested URL more expected (either "//" or "http://")
78-
const protocol = isNoProtocol // e.g. "./foo/bar.js" or "foo/bar.js" both converted to paths relative to root
79-
? ''
80-
: isRelativeProtocol // e.g. "//localhost:8081/foo/bar.js?platform=ios"
81-
? '//'
82-
: _tempProtocol + '//'; // e.g. "http://localhost:8081/foo/bar.js?platform=ios"
80+
if (isNoProtocol) {
81+
throw new Error(
82+
'Expecting the request url to have a valid protocol, e.g. "http://", "https://", or "//"',
83+
{cause: rawNonJscSafeUrlEncodedUrl},
84+
);
85+
}
86+
87+
const protocol = isRelativeProtocol ? '//' : _tempProtocol + '//';
8388

8489
const sourceUrl = jscSafeUrl.toJscSafeUrl(
8590
protocol + host + requestPathname + search + hash,

0 commit comments

Comments
 (0)