Customize common urls server side

[fflorent]

Context

The client and the server share the commonUrl object (currently declared here).

Though both the client and the server has to build this object on their side.

This leads to awkward pieces of code which tries to customize the URLS and where we need to guess whether we are client-side or server-side, and:

1. If we're server-side, read an env variable and if it does not exist return the default value.
2. If we're client-side, get the env variable passed through sendAppPage, read its value through the right property of gristConfig and return the default value if it does not exist

Example here

The problem is that we have to really take care that the client and the server build the same object on their side.

Proposed solution

This PR is an alternative implementation of #1728 in which we just let the server build the commonUrls and pass it to the client.

I hope it is much simpler and less error-prone.

Related issues

Has this been tested?

• 👍 yes, I added tests to the test suite
• 💭 no, because this PR is a draft and still needs work
• 🙅 no, because this is not relevant here
• 🙋 no, because I need help

[hexaltation]

Despite some blank lines introduced, LGTM.
Great job.
Much more simpler to handle for self hosters.
No more need to ask PR to expose one of these customUrls.

[hexaltation]

Suggested change

[fflorent]

This newline is intended, I prefer having a block for local modules (above) and dependencies + native modules below.

Though I won't mind much removing it if generally people prefer the code without it.

[manuhabitela]

Cool :)

Is their a particular reason to directly use window.gristConfig instead of getting the config through app/common/urlUtils.ts#getGristConfig? window.gristConfig is basically never directly used in the client code so I'm wondering.

[fflorent]

@manuhabitela Interesting… But it does not feel right to me.

I don't know what a function that should be executed client-side only does in app/common, it's quite confusing.

I personally prefer what I did, even though it could be improved (we could maybe create a function that would return window.gristConfig.commonUrls, that may save code).

Not much a strong opinion though.

[dsagal]

What do you think of defining app/client/lib/commonUrls.ts, to export commonUrl analogously to app/server/lib/commonUrls? Then this code wouldn't change, and more importantly, we'd encapsulate access to the global window object.

[dsagal]

BTW, is it not a problem that ICommonUrlsChecker makes every property required? Does it mean that you can't override only some, but must override all or none? This might be intentional, if you want to make sure you don't use any default values, but doesn't it interfere with upgrades whenever a new property gets added?

[fflorent]

Does it mean that you can't override only some, but must override all or none?

No. If you read the code above, the user-defined properties are merged with the default ones, and that's the result that is checked here.

Which means that it fails in either cases:

• defaultUrls object does not fully implement the ICommonUrls interface, but the error should occur during the build
• adminDefinedUrls add properties that is not defined in the ICommonUrls interface, preventing the server to start.

[dsagal]

Oh you are right, sorry. I guess the second point may still be an issue -- it would cause failures on rare occasions that a property is removed; also if you add a new property to your config for a new version, the older versions wouldn't start. ICommonUrlsChecker.check(merged) would accept extra properties.

[dsagal]

Generally, I think this approach is much better. But for us at least, it would force the biggish blob of ALL the URLs to get sent around with every page, whereas before most of it was cached as part of JS bundles.

It's not a serious problem to insist on optimizing it, but I think it would be fairly easy to keep defaults out of the gristConfig. I am thinking of defining all the defaults (no process.env or anything) in ICommonUrls, as defaultCommonUrls, and in server we could combine e.g.

export const commonUrlOverrides: Partial<ICommonUrls> = defaults({}, getJsonEnvUrls(), getOldEnvUrls());
export const commonUrls: ICommonUrls = defaults({}, commonUrlOverrides, defaultCommonUrls);

Then we'd only include commonUrlOverrides into gristConfig. And in the client side, we'd similarly do

export const commonUrls: ICommonUrls = defaults({},
window.gristConfig.commonUrlOverrides, defaultCommonUrls);