fix: inline origin resolution, drop original dependency - #281
Conversation
Sorry, something went wrong.
DaleGardner
left a comment
There was a problem hiding this comment.
👍
Sorry, something went wrong.
joeybaker
left a comment
There was a problem hiding this comment.
I've got a bit of a nit suggestion, but seems good!
Sorry, something went wrong.
| */ | ||
| function getOrigin (url) { | ||
| if (typeof url === 'string') url = parse(url) | ||
| if (!url.protocol || !url.hostname) return 'null' |
There was a problem hiding this comment.
IMO 'null' is a little confusing. It could be perceived as a bug? Maybe we should return 'invalid url'?
Sorry, something went wrong.
There was a problem hiding this comment.
It is confusing, but it is also in accordance with RFC6464, so I think I will leave it like this.
Sorry, something went wrong.
| * | ||
| * @param {String|Object} url URL to transform to it's origin. | ||
| * @returns {String} The origin. | ||
| * @api private |
There was a problem hiding this comment.
should leave @link https://github.com/unshiftio/original/blob/507b362269c0ad4405d095aedc227c40aecaf68a/index.js as credit to orignal code.
Sorry, something went wrong.
Apparently the original dependency depends on url-parse module which recently saw some security issues reported. We are no longer depending on this as of 2.0, but the 1.x range does. In order to minimize noise related to this vulnerability, this PR moves the few lines of logic we depended on from this module into the 1.x branch. This ensures we still support node 12 (and 10?).
If approved, I will update the changelog and release a new patch release on the 1.x range - but I am eager to call this the last fix I will be doing for the 1.x range - ideally I would like to move to node 14 as the lowest supported version, following node LTS.