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

CORS headers for client/index.js #893

Closed
wants to merge 3 commits into from
Closed

Conversation

seanmorris
Copy link
Contributor

@seanmorris seanmorris commented Dec 22, 2023

What is this PR doing?

This allows any domain to import() the client/index.js file from https://playground.wordpress.net/.

It restores the changes described in:
#873

More specifically, #773 exposed the client library on Playground.wordpress.net:

https://playground.wordpress.net/client/index.js

That PR was accompanied by a .htaccess update to ensure the correct Access-Control-Allow-Origin header was set – otherwise the index.js file cannot be import()-ed in JavaScript.

Unfortunately, that update was lost in a force-push. I patched it ad-hoc on the server, but we should ship the correct setup out of the box.

Luckily, GitHub still keeps that code change here:

a6f685d

What problem is it solving?

Without these changes the index.js file cannot be import()-ed in JavaScript when pulling the file from a different domain. This adds the set Access-Control-Allow-Origin "*" header that allow JS running on any domain to import() that file.

How is the problem addressed?

Other domains aren't able to import() the client/index.js file from https://playground.wordpress.net/ without CORS headers. Right now, the .htaccess file has been placed ad-hoc, which is a fragile change that is prone to being lost.

This codifies the publishing of the .htaccess file as a part of the build process, so it won't be lost or overwritten accidentally.

Testing Instructions

Checkout the branch and install deps:

git checkout sm-cors-headers-client-js
npm install

Run the build:

npx nx reset
npm run build

Ensure the following files exist & match the contents below:

dist/packages/playground/remote/.htaccess

AddType application/wasm .wasm
AddType	application/octet-stream .data

dist/packages/playground/website/.htaccess:

<FilesMatch "(client\/|blueprint-schema\.json)">
Header set Access-Control-Allow-Origin "*"
</FilesMatch>

dist/packages/playground/wasm-wordpress-net/.htaccess:

AddType application/wasm .wasm
AddType	application/octet-stream .data
<FilesMatch "(client\/|blueprint-schema\.json)">
Header set Access-Control-Allow-Origin "*"
</FilesMatch>

@seanmorris seanmorris self-assigned this Dec 22, 2023
@seanmorris seanmorris added [Type] Bug An existing feature does not function as intended [Aspect] Website labels Dec 22, 2023
@dmsnell
Copy link
Member

dmsnell commented Dec 22, 2023

Can we update the description to include some context on why these changes were lost and why it's safe now to add them back in?

It would also be good to expand the "What problem is this solving" section to note which conditions prevent index.js from being imported dynamically, as it would be my expectation coming across this that it isn't essential if served from the same origin as the Playground itself.

@seanmorris
Copy link
Contributor Author

Can we update the description to include some context on why these changes were lost and why it's safe now to add them back in?

It would also be good to expand the "What problem is this solving" section to note which conditions prevent index.js from being imported dynamically, as it would be my expectation coming across this that it isn't essential if served from the same origin as the Playground itself.

@dmsnell no problem, I'll get right on that.

@seanmorris seanmorris changed the title [DRAFT] Restoring CORS changes to .htaccess fies Restoring CORS changes to .htaccess fies Dec 27, 2023
@seanmorris seanmorris changed the title Restoring CORS changes to .htaccess fies CORS headers for client/index.js Dec 28, 2023
@seanmorris
Copy link
Contributor Author

@dmsnell How does that new description look?

@dmsnell
Copy link
Member

dmsnell commented Dec 29, 2023

@seanmorris I think it would be good to note that this is brining back a change that was unintentionally lost during a force-push, that it was never meant to be removed in the first place. this will let people know that the commit is a correction to an earlier mistake and not a more proper change in its own right.

once it's rebased, squashed, and the commit message is ready we can merge it.

@@ -19,7 +19,8 @@
"cp -r ./client ./wasm-wordpress-net/",
"cp -r ./remote/* ./wasm-wordpress-net/",
"cp -r ./website/* ./wasm-wordpress-net/",
"cat ./remote/.htaccess > ./wasm-wordpress-net/.htaccess"
"cp ../../../packages/playground/website/.htaccess ./website/",
Copy link
Collaborator

Choose a reason for hiding this comment

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

For posterity, we may need to reuse the same vite plugin as playground/remote in playground/website before merging this one:

	/**
	 * Copy the `.htaccess` file to the `dist` directory.
	 */
	{
		name: 'htaccess-plugin',
		apply: 'build',
		writeBundle({ dir: outputDir }) {
			const htaccessPath = path('.htaccess');

			if (existsSync(htaccessPath) && outputDir) {
				copyFileSync(htaccessPath, join(outputDir, '.htaccess'));
			}
		},
	} as Plugin,

@adamziel
Copy link
Collaborator

Superseded by #873

@adamziel adamziel closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Aspect] Website [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants