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

Adding blueprint.json for enabling plugin previews in WP.org plugin repo #242

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ashfame
Copy link
Member

@ashfame ashfame commented Jan 17, 2024

This PR moves the assets directory one level under .wporg so that blueprint generating script and setup php code can reside there.

The idea of the helper files (blueprint_runphp_step_code.php and update_blueprint.sh) is to easily manage the setup PHP code in an actual PHP file and using the bash script to update the runPHP step's code in blueprint.json.

Playground preview based on the blueprint file in this PR - https://playground.wordpress.net/?blueprint-url=https://raw.githubusercontent.com/automattic/chatrix/blueprint/.wporg/assets/blueprints/blueprint.json

@ashfame ashfame self-assigned this Jan 17, 2024
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.

Tested and looks good, great job!

Copy link
Member

@psrpinto psrpinto left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@psrpinto
Copy link
Member

psrpinto commented Jan 18, 2024

I figured it would not be too hard to make it possible to disable the service worker, so I done that in #243.

If we get that one merged first, you should then be able to do the following to disable the service worker:

<!-- wp:automattic/chatrix {"enableServiceWorker":false,"instanceId":"4986644425212020","defaultHomeserver":"matrix.org"} /-->

@ashfame
Copy link
Member Author

ashfame commented Jan 19, 2024

@psrpinto Awesome, lets get merged first and then I can make changes in this PR and hopefully the login would work now 🤞

@ashfame
Copy link
Member Author

ashfame commented Jan 19, 2024

Good news, the login worked with service worker disabled. 👌

Bad news, it still sometimes try to load the service worker and fails with "something went wrong .... sw.js load failed". Almost always does on popup page.

Here is the playground link with main branch of chatrix - https://playground.wordpress.net/?blueprint-url=https://blog.ashfame.com/a8c/blueprint.json

Another blocker would be that on page refresh, it retains the matrix logged in session. I believe, it shouldn't be the case before this goes live for users.

@psrpinto
Copy link
Member

The enableServiceWorker attribute only applies to the block, so indeed popup will still use the service worker.

Concerning, the session remaining logged-in after refresh, I would assume thats the expected behaviour, as that's also how the plugin works in "normal" conditions. Would it not be weird that the plugin works differently in the playground?

@ashfame
Copy link
Member Author

ashfame commented Feb 1, 2024

Concerning, the session remaining logged-in after refresh, I would assume thats the expected behaviour, as that's also how the plugin works in "normal" conditions. Would it not be weird that the plugin works differently in the playground?

I worry to stand out from the playground's perspective of "All your changes are private and gone after a page refresh." :)

And even if we are tying up with playground's db setting, we would still not be able to reliably wipe chatrix storage when playground wipes its storage.

So currently, this is on hold as I don't feel comfortable pushing this live as is. I would rather Chatrix lose everything upon reload rather than risk leaving an access token behind for a user when they just leave.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants