Page MenuHomePhabricator

[web] Remove intial redux state from website responders
ClosedPublic

Authored by michal on Sep 12 2023, 5:07 AM.
Tags
None
Referenced Files
F3386325: D9145.id31301.diff
Fri, Nov 29, 4:13 AM
Unknown Object (File)
Mon, Nov 25, 3:32 PM
Unknown Object (File)
Mon, Nov 25, 3:09 PM
Unknown Object (File)
Tue, Nov 19, 7:16 PM
Unknown Object (File)
Fri, Nov 8, 4:03 PM
Unknown Object (File)
Fri, Nov 8, 12:50 AM
Unknown Object (File)
Oct 23 2024, 2:24 PM
Unknown Object (File)
Oct 23 2024, 1:14 PM
Subscribers

Details

Summary

ENG-4752
Depends on D9141

After adding the call to initial redux state we can finally remove it from website-responders. We need to add a new default web state that will make sure the app runs while we make the call to the keyserver for the initial state.

Test Plan

Load the web app when logged in and when logged out. Check that the app works and there are no errors in the console.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

web/redux/default-state.js
84 ↗(On Diff #30963)

What about squadcal.org?

web/redux/default-state.js
84 ↗(On Diff #30963)

It's an interesting question. Today we use squadcal.org as the default on native. One advantage of this is that it avoids any using web.comm.app for both serving the web app and for my keyserver. Using squadcal.org here would make it easier to say "web.comm.app just serves the web app", which is perhaps a follow-up task to the "separate web and landing from keyserver" goal. However, I could imagine there might be CORS issues or something if a page loaded from web.comm.app is sending requests to squadcal.org.

web/redux/default-state.js
84 ↗(On Diff #30963)

There a task for CORS already (there isn't an explicit task for migrating to squadcall because I considered it a part of ENG-4756 but I haven't broken it into smaller tasks yet). In any case I think this should be done as separate to this diff.

This revision is now accepted and ready to land.Sep 15 2023, 3:27 AM
web/redux/default-state.js
82–84

This is a significant regression that makes it impossible to run a staging environment for the keyserver for testing purposes. It appears that your changes make it impossible to host the Comm web app on any domains other than web.comm.app

web/redux/default-state.js
82–84

Then again, not clear how this should work in a multi-keyserver world, where keyserver and web are separated...

web/redux/default-state.js
82–84

it impossible to run a staging environment for the keyserver for testing purposes

Are we doing something like this currently, or do you mean the general staging environment that we had been working towards?
After ENG-5153 (updating the urlfacts) this will be changed to point to squadcal so it's not obvious what would the staging web app point to. The easiest fix would be to just introduce some env variable with keyserver url or something like this.

But if it's not immediately needed, than this should get handled automatically in a multi-keyserver world if I understand everything correctly. There will be some discovery service that keyservers can register with, and from which clients can get information about available keyservers and connect to them (ENG-4135).

web/redux/default-state.js
82–84

Are we doing something like this currently,

Yes, sometimes I need to test things in a publicly-addressable way, so I've set up a mirror of Comm (with an empty database) at comm.domains. So far I've only had two cases where I've needed this: primarily testing SIWE, but also recently I wanted it for testing master against an old version of Safari via an online tool.

There will be some discovery service that keyservers can register with

We've talked about this, but heads-up that in terms of sequencing, I'm thinking of that one as a sort of a "follow-up" to the multi-keyserver launch... we could launch with the expectation that users input keyserver URLs manually.

The easiest fix would be to just introduce some env variable with keyserver url or something like this.

I think this would be a good idea, but we should use getCommConfig for it. In the cases I've listed above, I actually didn't need a staging keyserver, just a staging web app / landing page. It would've worked fine if not for the CORS issues that prevented the staging web app from loading (it crashes trying to access the prod keyserver).

We've talked about this, but heads-up that in terms of sequencing, I'm thinking of that one as a sort of a "follow-up" to the multi-keyserver launch... we could launch with the expectation that users input keyserver URLs manually.

Makes sense. This simpler version also works for the staging needs.

I think this would be a good idea, but we should use getCommConfig for it

I'm going to handle this regression as part of my remaining work for web separation. We can use the url facts to set the correct CORS and inject the urlPrefix as a variable into the html which the web app will use (so instead of having "dev" and "prod" setup it will be dynamic based on url facts).
This is a bit of a step-back because it again couples keyserver with web app, but we can easily remove it after it's not needed when we implement the keyserver selection (so user can just enter a keyserver URL).