new version #13
new version #13
Conversation
…ers for stronger typing. uses typescript & jest. uses WHATWG classes and drops node.js imports and 3rd-party deps.
|
I'm not seeing the new TS files. Did you skip adding them to the commit? |
|
Bah yeah sorry... they're on the branch now. |
|
|
||
| export class UrlGrey { | ||
| #url: string | ||
| _parsed: Parsed | null |
There was a problem hiding this comment.
is this intended to be public? it has an underscore but no visibility modifier
| const defaults = getDefaults() | ||
|
|
||
| this._parsed.protocol = this._parsed.protocol || defaults.protocol | ||
| return this._parsed |
There was a problem hiding this comment.
We should mark this as readonly in typescript, or copy it on the way out. readonly is probably better. if we don't do one of these then calling code can change the internal state by mutating the return value.
| throw new Error("file urls don't have ports") | ||
| } | ||
| const clone = this.clone() | ||
| clone.parsed().port = parseInt(`${num}`, 10) |
There was a problem hiding this comment.
ah I see you're relying on manipulating the internal state here. hmm. Maybe we should instead have a method called extend which takes a Partial<Parsed> merges it with the current instance's Parsed and produces a new UrlGrey from that.
| pathname: string; | ||
| port: number | null; // some protocols have no port, like file:// | ||
| path: string; | ||
| query: string; |
There was a problem hiding this comment.
any reason not to use URLSearchParams here for the query property?
| const p = urlParse(`http://localhost/${url}`) | ||
| clone.parsed().hash = p.hash | ||
| clone.parsed().query = p.query | ||
| return clone.path(p.pathname) |
There was a problem hiding this comment.
is there any rhyme or reason to which fields require you to go through this.parsed().xyz vs having a dedicated method like this.xyz()?
There was a problem hiding this comment.
URL feels like it already supports many of the features that urlgrey is adding on.
- would it make sense to have
_parsedbe a URL object that you can mutate and stringify? - does the library as a whole still make sense?
URLhas good documentation and is pretty easy to work with! If there are functions that URL is missing, would it make sense to set up helpers that take aURLobject and transform that object?
| }, | ||
| "maintainers": [ | ||
| "Gregg Caines <gregg@caines.ca> (http://caines.ca)" | ||
| ], | ||
| "dependencies": { | ||
| "fast-url-parser": "^1.1.3" |
There was a problem hiding this comment.
I'd recommend benchmarking this removal. There's a huge difference between it and the Node.JS url library and I'd be curious to see how things compare with URL. Using fast-url-parser significantly reduced how much event-loop-blocking we were seeing within API when handling 1,000-10,000s of urls.
port()is used for setting the port, butgetPort()is used for getting it. UrlGrey is more for url-manipulation than parsing, sosetPort()seemed too verbose and backward incompatible.