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

[Blueprints] Translate GitHub.com file URLs into CORS-accessible raw.githubusercontent.com #1810

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Sep 26, 2024

Translates GitHub URLs into raw.githubusercontent.com URLs.

For example:

{
	"steps": [
		{
			"step": "writeFile",
			"path": "/wordpress/wp-includes/version.php",
			"data": {
				"resource": "url",
				"url": "https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/version.php"
			}
		}
	]
}

Would be interepreted as

{
	"steps": [
		{
			"step": "writeFile",
			"path": "/wordpress/wp-includes/version.php",
			"data": {
				"resource": "url",
				"url": "https://raw.githubusercontent.com/WordPress/wordpress-develop/trunk/src/wp-includes/version.php"
			}
		}
	]
}

Motivation

There's virtually a zero chance you actually want to refer to the HTML response served
by GitHub.com, with the GitHub UI, file preview, etc. in it. Almost certainly, you want
to download the raw file.

This often confuses Blueprint authors when the GitHub URL they've used in their Blueprint
does not work. There's plenty of issues in the Playground repository asking specifically
about that. Well, GitHub.com response is not what they want, and even if it was, GitHub
does not provide the necessary CORS headers.

While the URL rewriting might confuse advanced developers, they're in a good
position to figure it out. This feature shouldn't do any harm.

Note the rewriting is implemented in UrlResource, which is used in all Playground
implementations, e.g. the browser, the CLI, Studio, etc. While most of them don't
need to worry about CORS, we still want to make sure the same Blueprints will work
in all Playground runtimes.

Testing instructions

Confirm all CI checks pass

Caveats

Directory URLs are not supported. For example, a URL such as

https://github.com/WordPress/blueprints/tree/trunk/blueprints

Would be rewritten to

https://raw.githubusercontent.com/WordPress/blueprints/trunk/blueprints

Which yields just 404: Not Found. I think that's fine. At least the error communicates more than just "missing CORS headers".

There's no way to distinguish between a file and a directory based just on its GitHub.com
URL. If this starts coming up a lot in Playground issues, let's explore consulting the
repository contents and rewriting the URL resource as a git directory resource.

@see #1793

cc @akirk @dmsnell

Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

I think this is a very reasonable and helpful improvement.

*/
if (this.resource.url.startsWith('https://github.com/')) {
const match = this.resource.url.match(
/^https:\/\/github\.com\/(?<owner>[^/]+)\/(?<repo>[^/]+)\/blob\/(?<branch>[^/]+)\/(?<path>.+[^/])$/
Copy link
Member

@akirk akirk Sep 26, 2024

Choose a reason for hiding this comment

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

I think it could be reasonable to limit the rewriting to known file extensions, for example:

Suggested change
/^https:\/\/github\.com\/(?<owner>[^/]+)\/(?<repo>[^/]+)\/blob\/(?<branch>[^/]+)\/(?<path>.+[^/])$/
/^https:\/\/github\.com\/(?<owner>[^/]+)\/(?<repo>[^/]+)\/blob\/(?<branch>[^/]+)\/(?<path>\.(?:php|js|json|zip|md|txt))$/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered that, but there's all sorts of extensions (e.g. .eslintrc) and lots of files without any extension out there. I'd rather do a blanket rule and deal with error 400 instead of CORS for directories, than arbitrarily rewrite only a subset of extensions.

@adamziel adamziel merged commit 8be9f83 into trunk Sep 27, 2024
5 checks passed
@adamziel adamziel deleted the understand-github-com-urls branch September 27, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants