releases : use native URL class - #6678
Conversation
|
looks good, not sure if the build worked though - https://travis-ci.org/devtools-html/debugger.html/builds/406901630?utm_source=github_status&utm_medium=notification. I can help take a look tomorrow |
|
I just pushed a small update that adds a utility for "safely" using |
|
OK, so I figured out the build issues that's been blocking me: This file expects the export from WebpackOptionsValidationError: Invalid configuration object. Webpack has been initialised using a configuration object that does not match the API schema.
- configuration misses the property 'entry'.
object { <key>: non-empty string | [non-empty string] } | non-empty string | [non-empty string] | function
-> The entry point(s) of the compilation. |
|
I started making some changes here but still no dice: https://gist.github.com/darkwing/1d15a4af7eadd9dcf8278b606cb79f57 Will get back to this later today or tomorrow. |
| } | ||
| }, | ||
| webpackConfig: buildConfig(envConfig) | ||
| }; |
There was a problem hiding this comment.
this seems weird. wouldn't we want to do externals in buildConfig
There was a problem hiding this comment.
Yes, it needs to be moved there, per my gist
| try { | ||
| return new URL(url); | ||
| } catch (err) { | ||
| return defaultUrl; |
There was a problem hiding this comment.
at this point, we should know we have URL
There was a problem hiding this comment.
Using new URL(input) is dangerous, as it can throw an error which would brick out our panel. It's the same bug I fixed for NetMonitor. Unless we have a URL validation process, I think this is a decent bet
There was a problem hiding this comment.
This also comes to mind: #5102, which would avoid having to do the default URL fields
There was a problem hiding this comment.
got it. then maybe we should do something like new URL(<minimal viable url>) which would help us avoid the default URL fields
|
I was thinking about this and I bet we can do two things here:
basically, we don't need any webpack stuff |
| export function parse(url: string): URL | object { | ||
| try { | ||
| const urlObj = new URL(url); | ||
| urlObj.path = urlObj.pathname + urlObj.search; |
There was a problem hiding this comment.
"path" isn't a native URL property -- I found this in URL's readme
| path: "/foo/bar/baz.js" | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Hmm, it'd still nice to make sure we don't parse too often.
perhaps we need to spy on ./utils/url parse. Jest has pretty good module mocking
There was a problem hiding this comment.
Normally when I do this kind of mocking I use jest.mock('module') and if I need to specifically mock one of its exports I can do const mockSpy = jest.fn(); jest.mock('module', () => ({ theExport: mockSpy }));
|
hmm, seeing some mochitest fails that might be worth digging in locally |
|
@wldcordeiro @bomsy Do you have any ideas here for restoring Jest spy functionality? I've tried a variety of items here (https://stackoverflow.com/questions/40465047/how-can-i-mock-an-es6-module-import-using-jest) and none seem to work. |
| group: "file://" | ||
| }; | ||
| } else if (host === null) { | ||
| } else if (!host) { |
There was a problem hiding this comment.
The url lib returned null when info didn't exist, whereas the native URL returns "". TIL.
Fixes Issue: #6645
Summary of Changes
Test Plan
I'm not sure I am doing it right. Please add a comment if you see something is missing, I'll take it into account.