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 :sweat_smile: 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 :confused: 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 :smile:

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 :wink:

bartosz me too, thanks :smile:>