Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make YoutubeLinkEnhancement URL parsing more robust. #4003

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

sengi
Copy link
Contributor

@sengi sengi commented Apr 30, 2024

URL API searchParams works pretty much everywhere since around 2017, which includes all the user agents we support. It also considerably simplifies the code and makes the intent much clearer.

(We have to avoid static canParse() though, because Samsung Internet is weird.)

Resolves https://github.com/alphagov/govuk_publishing_components/security/code-scanning/1.

No visual or functional change intended.

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4003 April 30, 2024 13:42 Inactive
@sengi sengi force-pushed the sengi/parse-urls-properly branch from 8346d0e to dcb2e38 Compare April 30, 2024 13:55
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4003 April 30, 2024 13:55 Inactive
@sengi sengi force-pushed the sengi/parse-urls-properly branch from dcb2e38 to 4621243 Compare April 30, 2024 14:17
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4003 April 30, 2024 14:17 Inactive
@sengi sengi force-pushed the sengi/parse-urls-properly branch from 4621243 to c8ecaa4 Compare April 30, 2024 14:28
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4003 April 30, 2024 14:29 Inactive
@sengi sengi marked this pull request as ready for review April 30, 2024 14:31
@sengi sengi requested a review from davidgisbey April 30, 2024 14:34
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sengi looks like a sensible change except that includes and (haven't confirmed this yet but fairly sure) searchParams aren't supported by older browsers like IE. While we don't support those browsers we do want to ensure they have a reasonable experience of GOV.UK and this change would probably cause all of the JS to break suddenly.

We're currently working on an RFC to remove JS support for IE11, can this PR wait until then? alphagov/govuk-rfcs#171

@sengi
Copy link
Contributor Author

sengi commented May 1, 2024

Ah ok, TIL we still de facto support IE11 😅

If you're sure we need to continue to support embedded YT videos in IE11 for the time being then I'm happy to stick this on the back burner until you're ready to drop IE.

I'll make it draft for now anyway.

@sengi
Copy link
Contributor Author

sengi commented May 1, 2024

Do we have a preferred polyfill library for this sort of thing? (Not necessarily suggesting it's worth it, just curious.)

@sengi sengi marked this pull request as draft May 1, 2024 11:10
@sengi
Copy link
Contributor Author

sengi commented May 1, 2024

Actually wouldn't it be just dropping support for adding/editing YT embeds in the content management tools in IE? Is this library even used in the public-facing frontends?

Ah no I was grepping wrong; it is at least pulled into some of the public frontends.

@sengi
Copy link
Contributor Author

sengi commented May 1, 2024

Even so, I'm not sure this would affect anything public-facing because it's in the govspeak library and surely we're not doing the govspeak->html transform on the client? 🤔

@andysellick
Copy link
Contributor

Even so, I'm not sure this would affect anything public-facing because it's in the govspeak library and surely we're not doing the govspeak->html transform on the client? 🤔

Correct, we're not doing the govspeak->html transform on the client, but we are running this JS on that output in the client to convert links to videos into embedded videos. See this page for an example - if JS is disabled the embedded videos show simply as links to Youtube.

@sengi
Copy link
Contributor Author

sengi commented May 2, 2024

Aha thanks! Super helpful example ❤️

@sengi
Copy link
Contributor Author

sengi commented Jun 18, 2024

I'm guessing this depends on #4000 — does that sound right?

[URL API] `searchParams` works in all browsers as of 2017. It also
considerably simplifies the code and makes the intent much clearer.

Resolves
https://github.com/alphagov/govuk_publishing_components/security/code-scanning/1

[URL API]: https://developer.mozilla.org/en-US/docs/Web/API/URL
@andysellick andysellick force-pushed the sengi/parse-urls-properly branch from c8ecaa4 to fbcdeb5 Compare September 3, 2024 15:14
@andysellick andysellick marked this pull request as ready for review September 3, 2024 15:26
@andysellick andysellick merged commit 41c7e28 into main Sep 4, 2024
12 checks passed
@andysellick andysellick deleted the sengi/parse-urls-properly branch September 4, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants