Should we return undefined or null?

[radiovisual]

Back when I first created this module, I used to just return an id String or undefined when an id was not found. Then, the request came in that the module returns the service provider as well, which made a lot of sense, so then there came a major version bump and the return values of an object like this:

When an id is found:

{
  id: String,
  service: String
}

This lets us do easy destructuring like this:

const { id, service } = getVideoId('https://some/video/url/with/id');

if (id) {
 // We have an id, do something with it
}

But what happens if no id is found?

When no id is found

Then we return an empty object {}, which can still be friendly to destructuring:

const { id, service } = getVideoId('https://no/id/is/found');

if (id) {
 // We don't have an id, this line won't be reached
} else {
 // No id was found...
}

What are your thoughts between undefined and and an empty object?

[radiovisual]

Would we ever run into this scenario where we can identify a provider, but not find a url? So the return object might look like:

{
   service: 'youtube',
   id: undefined
}

If cases like this are found quite a bit, then of course, an issue or PR should be opened, if there is an actual id expected, but in the case of a valid video url that doesn't have a video id to extract, this return would make sense.

[ArnasDickus]

Would we ever run into this scenario where we can identify a provider, but not find a URL?

Did it ever happen? Because id's at least on youtube are pretty standard and will always be found. Also, I'm happy with an empty object instead of undefined, because of the reasons you mentioned.

[radiovisual]

Did it ever happen? Because id's at least on youtube are pretty standard and will always be found. Also, I'm happy with an empty object instead of undefined, because of the reasons you mentioned.

@ArnasDickus It could happen. The most prominent case is when you feed get-video-id a valid url that doesn't contain an id, for example, youtube's terms of service url:

const { id, service } = getVideoId('https://www.youtube.com/static?template=terms');
// => { service: 'youtube', id: undefined  }

which is not really a problem. If we ever find that get-video-id is not finding the id in a valid video url which contains a video id, then of course, we need to update get-video-id.

[ArnasDickus]

I see. Well for my use-case. That's not a problem, since I will be fetching predefined youtube URLs from the database. Which will definitely have an id. I'm good either way and have little opinion on this particular matter. I would say let's not fix it what isn't broken.

[radiovisual]

Some more thoughts on this came up recently in this PR: #52

[4lejandrito]

Hi @radiovisual,

I didn't know about this discussion. I'll add my thoughts after what you said in #52.

You make good points. Then probably the easiest way forward to have usable type definitions is this:

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

This can encode all potential scenarios:

1. URL recognized and valid id present: both service and id defined.
2. URL recognized but id not present: service is defined but id is null. I see value on this use case for malformed URLs for a given service. The calling application could then show a nice help message telling the user how the URLs for that service should look like.
3. URL not recognized: both service and id are null.

However this types would allow for an impossible case where the service is null but the id is defined, which makes no sense. The most accurate solution (even though it would be more complex) is this one:

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

In this case, when the returned value is not null we ensure the service is always defined and we avoid that edge case I mentioned. When the URL is not recognized, we return null.

I am happy to update my pull request to reflect the case you decide to go for.

Thanks a lot!

[radiovisual]

@4lejandrito My vote would be for the types to look like this:

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

I am OK with the fact that these type definitions support the impossible case where an id could be found while the service could not, because I don't think that it's the type definition's job to defend the combination of expected (or unexpected) outcomes of the data (or the relationships between the data fields), instead, it's job is to declare what data type each field should hold. So if we look at each field one-by-one, this type declarations above are accurate.

I also really want to avoid returning null or undefined in the case that neither an id or service could be found, because then we totally break the possibility to use destructuring:

const fn1 = () => undefined;
const { id, service } = fn1();
// TypeError: Cannot destructure property 'id' of 'fn1(...)' as it is undefined.


const fn2 = () => ({});
const {  id, service  } = fn2();
// id => undefined, service => undefined

If anyone ever encounters this impossible case where an id is found but a service is not, then hopefully they open a PR or report the bug. 😄

[4lejandrito]

@radiovisual sounds reasonable.

I'll update my PR today or tomorrow if that's ok.

Thanks!