Returning null for unrecognized urls

[4lejandrito]

Is this a feature, improvement or a bug fix?

Improvement.

If you changed any JavaScript files, did you write unit tests?

Yes.

Please describe (in detail) the solution(s) provided by your Pull Request

This is specially useful for Typescript use. With the current typings I get a type error when trying to do things like:

const {id, service} = getVideoId(url)
const videoId = getVideoId(url)?.id
const videoService = getVideoId(url)?.service

Also I believe null is a better return for unsupported URLs than an empty object.

An alternative to this would be to modify just the typings to:

export default function getVideoId(
	url: string
): {
	id?: string;
	service?: "youtube" | "vimeo" | "vine" | "videopress";
};

But that wouldn't be entirely accurate because it would imply that either the id or the service can be null or undefined independently. My proposal ensures that when the url is recognized both are defined.

[coveralls]

Coverage remained the same at 100.0% when pulling 2161f23 on 4lejandrito:improve-typings into 9616cf1 on radiovisual:master.

[rhutchison]

Please merge this, the last change kills me.

[radiovisual]

@4lejandrito Thanks for this! I have been thinking a lot more about this lately. So I appreciate the help. A couple of things to clarify though:

Also I believe null is a better return for unsupported URLs than an empty object.

Your PR only covers this case when the url is not recognized as a youtube, vine, videopress or vimeo url, for example:

const video = getVideoId('https://not.supported/video/url');
// => null

but in cases where it can still detect that the url belongs to one of these services youtube, vine, videopress or vimeo, then you would still be returning undefined for id when an id can't be found. Consider this test case, that would fail in your PR:

test('returns null for unrecognized vimeo urls', t => {
	t.is(fn('https://player.vimeo.com/unknown').service, 'vimeo'); // passes
	t.is(fn('https://player.vimeo.com/unknown').id, null); // fails with AssertionError: undefined === null
});

So from your proposed changes to the types:

export default function getVideoId(
	url: string
): {
	id?: string;
	service?: "youtube" | "vimeo" | "vine" | "videopress";
};

It could make sense to allow either/or the id or the service to be null or undefined in these situations where the service is recognized, but an id cannot be found.

And then there is this case to consider when neither the service or id can be found, do we return:

{
  id: null,
  service: null
}

Or simply return null instead of the object?

These different cases can confuse things on the receiving end of the function, because we would need to account for either null, in place of an object, or null/undefined values if the object exists.

Trying to find the best way to handle all cases is why I opened this discussion: #47

Happy to keep discussing this.

[radiovisual]

@4lejandrito my thoughts on the type defs are found in the discussion here: #47 (comment)

[4lejandrito]

Hi @radiovisual,

I push forced with the changes discussed in the thread.

Regards!

[radiovisual]

Awesome, thanks! ❤️ I will merge this and publish a new version soon

[radiovisual]

Version 3.1.9 has been published with these changes. Thanks again!