refactor: remove immutable from 'config' state slice

[smashercosmo]

Summary

Continue with #3853

Test plan

Run existing tests

[smashercosmo]

@erezrokah why do you think it's better to not use real default state? When tests are converted to typescript we'll have to either put a lot of tsignore statements or "as SomeType" statements.

[erezrokah]

I worried this will make the tests brittle. Personally I like to have in the tests as little as information possible required for them to pass so I don't have to change them too often.

As for typing the tests - we'll have to see if that makes our lives easier or not? I would expect tests to fail when something breaks and not rely on types?

[smashercosmo]

I would expect tests to fail when something breaks and not rely on types

this is true. On the other hand you're basically passing incorrect values to functions being tested. So tests might pass because of that. And might fail if correct values are provided.

[erezrokah]

this is true. On the other hand you're basically passing incorrect values to functions being tested. So tests might pass because of that. And might fail if correct values are provided.

I believe that's something to verify when writing the tests. The test should be scoped to assert a specific thing and not verify that backend.ts methods work with the default config?

If so, we should have separate unit tests to verify that backend.ts handles the configuration properly (and not assume it works because we're passing default values).

[smashercosmo]

Alright

[smashercosmo]

I'm not sure this is a correct way to handle it. Because here we're lying to compiler.

[erezrokah]

Under this function that value is always defined.
The only reason we allow not to set media_folder is if you have an external media library which in that case this code doesn't run.

[smashercosmo]

I'm wondering if there is more correct way to express it with types, so we don't have to use these escape hatches.

[smashercosmo]

Can't agree that using map + fromEntries is more readable than simple loop :) functional style doesn't really shine here and looks way more confusing than the loop.

[erezrokah]

Actually both methods have a bug of not maintaining the order of the collections as appeared in the configuration.
See c0e43bc

[smashercosmo]

Yeah, in future we can use js Map to maintain the order

[smashercosmo]

But I'm wondering, why is it happening in the first place. I thought modern js engines maintain property order in js objects. It wasn't the case in the past, but I was sure that it had been fixed.

[smashercosmo]

Yeah, I'm pretty sure this is the problem with Immutable. This works correctly. The order is being preserved.

let obj = {}; 
let arr = ['collection_1', 'collection_3', 'collection_2']
for (const item of arr) {
  obj[item] = item;
}

Though the order is not being preserved if property name is a number.

[erezrokah]

Yes, JS keeps insertions order. It's the fromJS that doesn't know how to handle it.

[erezrokah]

🔥

Sorry for taking so long to do this. I feel like I had to grow another set of 👀 for it.

[smashercosmo]

thx @erezrokah. no need to apologize. I know it's a lot of changes to review. thank you for being a thorough reviewer. I'll try to not make that many changes in future PRs, though it's not that easy in this particular-getting-rid-of-immutable case :)