Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OT Integration - Part 2 #305

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

OT Integration - Part 2 #305

wants to merge 2 commits into from

Conversation

Varun-Sethu
Copy link
Member

No description provided.

Copy link
Contributor

@zax-xyz zax-xyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so true frfr ong

frontend/src/packages/editor/operationManager.tsx Outdated Show resolved Hide resolved
frontend/src/packages/editor/operationManager.tsx Outdated Show resolved Hide resolved
frontend/src/packages/editor/operationManager.tsx Outdated Show resolved Hide resolved
frontend/src/packages/editor/operationManager.tsx Outdated Show resolved Hide resolved
}

const normalizeCustomTextPath = (contentBlock: CustomText, op: NodeOperation | TextOperation): number[] => {
// TODO: For Mae :P - So the issue here is that fields on javascript objects are unordered, this is unlike Go where (typically) the order in which
Copy link
Contributor

@zax-xyz zax-xyz Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actual unironic comment: this isn't quite correct, objects do maintain some order:

The traversal order, as of modern ECMAScript specification, is well-defined and consistent across implementations. Within each component of the prototype chain, all non-negative integer keys (those that can be array indices) will be traversed first in ascending order by value, then other string keys in ascending chronological order of property creation.

(MDN for..in, this also extends to object methods like Object.keys() and Object.entries())

Copy link
Member Author

@Varun-Sethu Varun-Sethu Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thats convenient, so should it directly correspond to the the order declared in the TS type? IE in this example:

type field = {
   a: string
   b: string
   aca: string
}

const x = { b: "hello", a: "world", aca: "there" };
const y = { a: "hello", b: "world", aca: "there" };

would x and y produce the same order for keys?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thats convenient, so should it directly correspond to the the order declared in the TS type?

Wait ig not since it depends on order of declaration in the prototype so ig anyone can declare it in any order they want

Copy link
Contributor

@zax-xyz zax-xyz Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessarily as declared in the type, as those aren't available at runtime - if you do something like send an json object from the backend and parse that from the frontend, it should maintain whatever order it was sent in (assuming there's no integer keys)

Copy link
Member Author

@Varun-Sethu Varun-Sethu Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm issue is though is that slate doesn't actually seem to give us a guarantee on order (for starters since it doesn't surface the entire object that its updated and only surfaces the field) because at the very least in Go I can just use reflection to map fields to their declaration order which doesn't seem possible at all in JS given each instance of a type could have a different field order

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gg rip that's doomed then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants