Fix: Preserve identity IDs when IDT is disabled #1359
Conversation
pastuxso
force-pushed
the
lifecycle-id-fixes
branch
from
647e4a3
to
ae66460
Compare
pastuxso
changed the title
|
@degrammer @pastuxso I'll be doing the review on this. Please don't request reviews from one the more people at once or jump in unprompted. Context and duplication of work is costly. |
pastuxso
force-pushed
the
lifecycle-id-fixes
branch
from
ae66460
to
0aab552
Compare
pastuxso
requested review from
degrammer and
sourishkrout
and removed request for
sourishkrout and
degrammer
|
| return | ||
| } | ||
| if (cell.metadata?.['id']) { | ||
| cell.metadata['runme.dev/originalId'] = cell.metadata['id'] |
There was a problem hiding this comment.
Don't think this is the right solution. It tries to fix "the issue" at the tail end of serialization instead of deserialization where it seems to originate.
I believe this code here's not "smart enough" yet. It ignores both identity requirements and the fact that runme.dev/id is not set which means cell identity wasn't required in the first place.
https://github.com/stateful/vscode-runme/blob/lifecycle-id-fixes/src/extension/serializer.ts#L319-L324
There was a problem hiding this comment.
One could argue that if runme.dev/id is not set the deserialization was requested without cell identity. One problem I could think of is that "newly inserted cells" through the notebook UX will likely not be treated correctly and go without ID. Checking the identity requirement directly should get around this issue.
| @@ -355,6 +355,11 @@ export abstract class SerializerBase implements NotebookSerializer, Disposable { | |||
| } | |||
|
|
|||
| data.cells.forEach((cell) => { | |||
| if (identity === RunmeIdentity.UNSPECIFIED && cell.metadata?.['runme.dev/originalId']) { | |||
There was a problem hiding this comment.
We should try to make reconcileCellIdentity go away instead of further expanding the quick fix band-aid that it was.
|
Closing as obsolete now that #1449 is merged. |
The issue is that when IDT is turned off, identity IDs need to remain unchanged, even if they are included in a notebook.
This includes:
runme.dev/stageId: This is a new metadata field used to transfer the IDT during serialize/deserialize requests only when RunmeIdentity is UNSPECIFIED.
Updates to assertDocumentContains to only match a portion of the document.
Test updates
Closes: #1354