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

Enable New Loader by default #6337

Closed
wants to merge 1 commit into from
Closed

Conversation

takahirox
Copy link
Contributor

@takahirox takahirox commented Oct 18, 2023

Related #6325

This commit enables the new loader by default. Also, adds a way to opt-out the new loader.

Changes

  • Enable the new loader by default
  • Replace "newLoader" URL query with "oldLoader" to force to use the old A-Frame based implementation for testing
  • Replace APP.hub.user_data.hubsUseNewLoader with APP.hub.user_data.hubsUseOldLoader to disable the new loader at room-level.
  • Update shouldUseNewLoader() to use "oldLoader" and APP.hub.user_data.hubsUseOldLoader

TODO (This PR or another PR)

This commit enables the new loader by default. Also, adds a way
to opt-out the new loader.

Changes

* Enable the new loader by default
* Replace "newLoader" URL query with "oldLoader" to
  force to use the old A-Frame based implementation for testing
* Replace APP.hub.user_data.hubsUseNewLoader with
  APP.hub.user_data.hubsUseOldLoader to disable the new loader
  at room-level.
* Update shouldUseNewLoader() to use "oldLoader" and
  APP.hub.user_data.hubsUseOldLoader
@takahirox takahirox requested review from netpro2k and keianhzo and removed request for netpro2k October 18, 2023 22:16
@takahirox takahirox added P1 Address as quickly as possible new-loader labels Oct 18, 2023
@keianhzo
Copy link
Contributor

Looks good to me. Are you planning to add the UI here on another PR?

I think it would also be good to reload all clients when that flag is updated by the admin. Do you know if that user_data update is triggering any event that we can use to reload all clients? Maybe we need to add it to reticulum. Probably for another PR.

@takahirox
Copy link
Contributor Author

Update: We don't merge this PR soon.

As we have been discussing internally, we changed a plan a little bit.

New plan

  • We first ship the new loader code but we make if disabled by default. And we add a UI (maybe in a room-level setting) to toggle the new loader. Users who want to try the new loader can opt-in it by themselves
  • Later at somepoint, we may think of enabling the new loader by default when we feel confident about the new loader. We keep the toggle UI. Users who encounter new loader issues can opt-out the new loader by themselves.

Are you planning to add the UI here on another PR?

Yes, I'm making another PR for adding the toggle UI. Once the toggle UI PR will be merged, we can ship the code. After the code shipment, when we will feel confident about the new loader we can enabled it by default by merging this PR.

I think it would also be good to reload all clients when that flag is updated by the admin. Do you know if that user_data update is triggering any event that we can use to reload all clients? Maybe we need to add it to reticulum. Probably for another PR.

I'm thinking the same. Ideally all the clients in a room should reload the page when the new loader is toggled. But I don't really want to change the reticulum code now in this very tight schedule. I'm checking through the client and reticlum code to try to find API or systems that triggers reload the page on a certain event and we can reuse. Probably the client reload PR would be another PR from the toggle UI PR.

In short, we need two or three PRs related to new loader enable/disable.

By the initial new loader release

  1. Add a new UI to toggle the new loader bitECS: Add a UI to toggle the new loader #6325
  2. (Optional?): Reload all the clients in a room when the new loader is toggled

After the initial release, by when we will enable the new loader by default

  1. Enable the new loader by default (This PR)

@takahirox
Copy link
Contributor Author

Regarding this

(Optional?): Reload all the clients in a room when the new loader is toggled

If I'm right it seems to be possible in Hubs Phoenix channel "hub_refresh" event listener in src/hub.js because

  1. Client pushes "update_hub" to Reticulum when room settings are updated
  2. Reticulum broadcasts "hub_refresh" to all the clients in the room

So Hubs Client can check if new loader activation flag is changed and invoke the page reload if it happens.

I will make a PR...

@takahirox
Copy link
Contributor Author

We have released the initial new bitECS based client that is disabled by default. We will think of enabling it by default when we will feel confident about the new client stability.

Closing this PR for now because it's already old. Let's make a new one when we will enable it by default.

@takahirox takahirox closed this Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-loader P1 Address as quickly as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants