vigetlabs / colonel-kurtz

A Block Editor
MIT License
319 stars 33 forks source link

YouTube regex might need updating #111

Open averyvery opened 8 years ago

averyvery commented 8 years ago

Current YouTube regex is new RegExp('(?:youtube(?:-nocookie)?\.com/(?:[^/]+/.+/|(?:v|e(?:mbed)?)/|.*[?&]v=)|youtu\.be/)([^"&?/ ]{11})', 'i')

...but I think there's a mistake in there. Escaped characters, since they're in a string, should be double-escaped like so:

new RegExp('(?:youtube(?:-nocookie)?\\.com/(?:[^/]+/.+/|(?:v|e(?:mbed)?)/|.*[?&]v=)|youtu\\.be/)([^"&?/ ]{11})', 'i')

Example: http://jsfiddle.net/averyvery/xwbt5u3L/

Not a big deal, though; I'm sure this doesn't break any existing functionality.

nhunzaker commented 8 years ago

Cool. Seems like it would pull out the video id regardless.

The test coverage for this is here: https://github.com/vigetlabs/colonel-kurtz/blob/master/addons/youtube/__tests__/youtube.test.jsx#L30-L59

I can't get to it right away, if you want to check it out now. Otherwise I can totally dig into this tomorrow or wednesday

averyvery commented 8 years ago

I don't see any reason to take care of it any time soon.

And yeah, it'll pull out the id either way; I can't think of a real case where this would cause a full failure. I was just copying a bit of it for my Vimeo module and realized the dots chunk weren't working as expected.

nhunzaker commented 8 years ago

Haha. it would be good to have some sort of error that checks the domain and says something like "The URL provided is not from YouTube..."

I'm less worried about the actual URL and more that it properly gets the video they want. We don't save the URL at all -- just the video ID

averyvery commented 8 years ago

Yeah, I see how it works — nbd, sorry if this is just wasting time. I was hoping it would be a little todo to be taken care of at some far future date.

nhunzaker commented 8 years ago

No problem at all! Talk is cheap. This would be a nice enhancement