Synchronous catalog config from a global var + types

[nl0]

In order to simplify access to the config data throughout the app (to a simple import cfg from 'utils/Config') and setup of some tools (such as sentry),
I've decided to refactor the config system to use a synchronous JS file which sets a global variable that later can be accessed by the app code.

This requires a change in deployment which will come soon (later today).

UPD: Actually, deployment change is only relevant for OPEN, so we can merge this PR without waiting for that change.

In order for local catalog instance to continue functioning, one should rename config.json file in static-dev folder to config.js and prepend the contents with window.QUILT_CATALOG_CONFIG =

TODO

• changelog

[codecov]

Codecov Report

Merging #3166 (53d3068) into master (6d50ef5) will decrease coverage by 0.13%.
The diff coverage is 42.22%.

@@            Coverage Diff             @@
##           master    #3166      +/-   ##
==========================================
- Coverage   35.12%   34.98%   -0.14%     
==========================================
  Files         660      661       +1     
  Lines       28964    28957       -7     
  Branches     4241     4239       -2     
==========================================
- Hits        10174    10132      -42     
- Misses      17609    17639      +30     
- Partials     1181     1186       +5     

Flag	Coverage Δ
api-python	90.72% <ø> (ø)
catalog	8.22% <42.22%> (-0.22%)	⬇️
lambda	86.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
catalog/app/constants/config.ts	0.00% <0.00%> (ø)
catalog/app/embed/Embed.js	0.00% <ø> (ø)
catalog/app/embed/debug-harness.js	0.00% <ø> (ø)
catalog/app/utils/LinkedData.tsx	0.00% <0.00%> (ø)
catalog/app/utils/Config.ts	51.35% <51.35%> (ø)
catalog/app/utils/sagaTools.js	0.00% <0.00%> (-53.34%)	⬇️
catalog/app/utils/ResourceCache.js	0.00% <0.00%> (-19.15%)	⬇️
catalog/app/utils/reduxTools.js	45.45% <0.00%> (-3.04%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[nl0]

Actually, deployment change is only relevant for OPEN, so we can merge this PR without waiting for that change.

[fiskus]

Sorry, I wasn't clear about getter

I thought about something like this:

// utilsConfig.ts

const config = readAndValidateConfigOnStart()
export default function getConfig() {
  return config
}

// some-module.ts
import getConfig from 'utils/Config'

const { noDownload } = getConfig()

It's almost the same as in the previous iteration (import cfg from 'utils/Config'), but instead of importing object you can import getter function.
Using getter function instead of object is more future-proof, because you can provide some options (ex. environment, context) to that getter, and it involves less rewriting.

[fiskus]

If your last iteration (590ea3a) is a trade-off for my feedback, then it's better to revert it, because I like previous iteration more, and, probably, you too

[nl0]

Sorry, I wasn't clear about getter

I thought about something like this:

// utilsConfig.ts

const config = readAndValidateConfigOnStart()
export default function getConfig() {
  return config
}

right now the code looks almost exactly like this

// some-module.ts
import getConfig from 'utils/Config'

const { noDownload } = getConfig()

It's almost the same as in the previous iteration (import { cfg } from 'utils/Config'), but instead of importing object you can import getter function.

you can do that with the current implementation if you need (via getGlobalConfig() or useConfig() functions)

Using getter function instead of object is more future-proof, because you can provide some options (ex. environment, context) to that getter, and it involves less rewriting.

i don't quite get that argument, please elaborate on this. what would some actual examples of this look like?
using a callable every time "just because" makes the code lengthier for no reason (you need to declare a new variable to assign the result of the call every time, and you also need to cache the result)

[nl0]

If your last iteration (590ea3a) is a trade-off for my feedback

i've addressed your feedback (added desktop flag and refactored printing logic a little bit) and did some refactoring to isolate side-effects (make utils/Config module side-effects-free and move effectful calls into a new module)

then it's better to revert it, because I like previous iteration more

why?

and, probably, you too

nope

[fiskus]

right now the code looks almost exactly like this

The main difference. In my pseudocode config validated and created only once, and then you can use the same object over and over

[fiskus]

Ok, I see.
I talked about getter, and then I saw you made export getConfig and export getClobalConfig, and thought it's addressing to my feedback. But it's not what I meant.

Now I see, that you actually ignored my feedback on getter, and that's kinda ok :)

[fiskus]

If utils/Config will be used only for constants/config - I agree with new changes

I care more about API, then implementation, because implementation is a subject to change, but API is what should be more or less stable in collaborative code. API wasn't clear for me

If you want to replace all:

import * as Config from 'utils/Config'
const { noDownload } = useConfig()

to

import cfg from `constants/config`
const { noDownload } = cfg

or

import { noDownload } from `constants/config`

then I OK with that, though I would prefer

import getConfig from `config`
const { noDownload } = getConfig()

[nl0]

right now the code looks almost exactly like this

The main difference. In my pseudocode config validated and created only once, and then you can use the same object over and over

useConfig does exactly that

[nl0]

I talked about getter, and then I saw you made export getConfig and export getClobalConfig, and thought it's addressing to my feedback. But it's not what I meant.

Now I see, that you actually ignored my feedback on getter, and that's kinda ok :)

i haven't ignored it and implemented that logic in useConfig function (tho i didnt make it the primary api)

If utils/Config will be used only for constants/config - I agree with new changes

yeah it will (after we replace all the useConfig calls)

I care more about API, then implementation, because implementation is a subject to change, but API is what should be more or less stable in collaborative code. API wasn't clear for me

ok makes sense

If you want to replace all:

import * as Config from 'utils/Config'
const { noDownload } = useConfig()

to

import cfg from `constants/config`
const { noDownload } = cfg

yeah more or less

or

import { noDownload } from `constants/config`

that's not quite feasible due to how webpack and ES modules work

then I OK with that, though I would prefer

import getConfig from `config`
const { noDownload } = getConfig()

ok i'm glad we can more or less agree on the api surface, but i still don't see a compelling reason to keep the extra unnecessary step of calling the function instead of just accessing the result directly (tho i'm still open to being convinced otherwise)

[fiskus]

Future needs for refactoring will convince you :)