Review chart migration code

[ptbrowne]

I hit a problem while working on migration. It might be indicative of something wrong with how we structure migrations, we may have to use migrationprops: current(draft) instead of migrationProps: draft in versioning.ts.

The problem can be reproduced by nesting two migrations:

const state = migrateConfiguratorState({
  chartConfig: migrateChartConfig()
})

And we get a "Cannot perform get on a proxy that has been revoked".

Fortunately right now, it does not seem to be a concern in the usual application code, just something we may have to watch out for.

Relevant discussion:

bartosz
maybe there's some reducer action before migration, that returns a proxy incorrectly? and then when it tries to migrate we have this error

patrick
I do not know why but I tried to have migrateConfiguratorstate and a migrateChartConfig inside since I believed that I need to update both
when I tried to do that, I had the error
I removed the inner migrateChartConfig and it was working

bartosz
ah
yes, migrate configurator state also migrates chart configs

patrick
the migrateChartConfig is done inside I see now
but not sure why it would fail

bartosz
that's why we need to bump configurator state version even if we only update chart config migrations
not ideal probably 😅
I am also not 100% sure why some immer things fail, I guess produce was called twice somehow? (edited)

patrick
yes but what I do not get is that produce should guard us from making mistakes since it should keep immer invisible to the outside
but it seems here that we leak somehow immer
I wonder where

bartosz
every time something fails with immer I am a bit in the dark, I think I don't understand it fully 😕 nor sure how it can leak

patrick
yes, a bit same for me
what I understand is that when you modify draft, you are in fact only creating a recipe for changes, that are applied, only when you finish the draft, and produce hides this createDraft / finishDraft process for you
but I guess that somewhere you have produce(draft => toto.chartConfig = draft), then it's the reference to the draft that is saved, not the resolved object

bartosz
aaa, maybe :thinking_face: makes more sense now
😄

patrick
myabe the migrationProps: draft should be migrationProps: current(draft)
anyway, I will keep it like that at the moment, it helped me already to lay it out in writing 😉

bartosz
me too, thanks 😄>