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

feat: Add unique ids for each block #1493

Closed
wants to merge 2 commits into from
Closed

Conversation

cobbcheng
Copy link
Contributor

@cobbcheng cobbcheng commented Dec 30, 2020

When editing documents collaboratively, we usually need a unique id to save and identify the current block. This PR is modified and resubmitted based on this #1333 , because this PR is no longer maintained

I'm adressing #873 with this PR.

Tested Features:

  • Existing data: If your previous block does not have an "id" property, it will create a new unique id
  • Existing data: If your previous block does have an "id" property, it will keep the existing unique id
  • Existing data: If a block gets changed the "id" property stays the same
  • Insert New Block: It will create a new unique id
  • Copy & Paste blocks: It will create a new unique ids per copied block
  • Move Blocks (Up/Down): It will keep the existing unique id

Important Note:

If you "transform/transition" an existing block to another block it will receive a NEW unique id

@neSpecc
Copy link
Member

neSpecc commented Jan 1, 2021

Please, add a description to the PR. What case you're trying to cover?

@cobbcheng
Copy link
Contributor Author

Please, add a description to the PR. What case you're trying to cover?

I'm sorry this is my negligence, I have added a description

@mahono
Copy link

mahono commented Jan 9, 2021

Awesome. This is exactly what we need to finally use EditorJs for our system. We also need an ID per block for collaboration features and in some special cases to map (save) each block to a separate database record (which is not possible without an identifier). UUIDv4 perfectly fits. Maybe someone needs another "id generator" but this could be added/opened in a later step. So I can't wait to see this feature in EditorJS. 👍

@chumbalayaa
Copy link

Yes, we'd love to have uids assigned for each block. Really important for our commenting use case!

@@ -14,6 +14,14 @@ function BlockAPI(
block: Block
): void {
const blockAPI: BlockAPIInterface = {
/**
* Tool id
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Tool id
* Block id

@@ -27,6 +27,11 @@ import SelectionUtils from '../selection';
* Interface describes Block class constructor argument
*/
interface BlockConstructorOptions {
/**
* Tool's id
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Tool's id
* Block's id

@@ -107,6 +112,11 @@ export default class Block {
};
}

/**
* unique identifier
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* unique identifier
* Block unique identifier

@@ -5,6 +5,11 @@ import {SavedData} from '../data-formats';
* @interface BlockAPI Describes Block API methods and properties
*/
export interface BlockAPI {
/**
* Tool name
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Tool name
* Block unique identifier

*
* @returns {string}
*/
export function generateUuidv4(): string {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to name this method just as generateId() and make UUIDv4 "private". E.g.

export function generateId(): string {
    return uuidv4();
}

function uuidv4(): string {
  // your code
}

so that we could change the id generator algorithm

@@ -759,7 +759,7 @@ export default class Paste extends Module {
*
* @returns {void}
*/
private insertEditorJSData(blocks: Array<Pick<SavedData, 'data' | 'tool'>>): void {
private insertEditorJSData(blocks: Array<Pick<SavedData, 'id' | 'data' | 'tool'>>): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

id seems unused.

Suggested change
private insertEditorJSData(blocks: Array<Pick<SavedData, 'id' | 'data' | 'tool'>>): void {
private insertEditorJSData(blocks: Array<Pick<SavedData, 'data' | 'tool'>>): void {

@neSpecc
Copy link
Member

neSpecc commented Feb 4, 2021

I think it's better to use nanoid(10) instead of UUIDv4

https://github.com/ai/nanoid

@hata6502
Copy link
Contributor

hata6502 commented Feb 4, 2021

Should we close the ancestor PR #1333 with respects?

@hata6502
Copy link
Contributor

hata6502 commented Feb 4, 2021

I think it's better to use nanoid(10) instead of UUIDv4

https://github.com/ai/nanoid

nanoid(10) is not recommended because of the collision rate.
The default behavior nanoid() is as safe as uuid.

https://zelark.github.io/nano-id-cc/

@neSpecc
Copy link
Member

neSpecc commented Feb 4, 2021

I think it's better to use nanoid(10) instead of UUIDv4
https://github.com/ai/nanoid

nanoid(10) is not recommended because of the collision rate.
The default behavior nanoid() is as safe as uuid.

https://zelark.github.io/nano-id-cc/

The uniqueness of block ids will be scoped by the single entry. So I think that collision of ids of blocks inside one entry will be rare.

@cobbcheng
Copy link
Contributor Author

Thanks for your suggestions, you are right, I will modify and submit again, thanks!

* @returns {string}
*/
export function generateBlockId(): string {
return uuidv4();
Copy link
Member

Choose a reason for hiding this comment

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

Check this comment please #1493 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok i will change to nano(10)

Copy link

Choose a reason for hiding this comment

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

@cobbcheng need any help on this?

// the project's config changing)

/**
* @type {Cypress.PluginConfig}
Copy link

Choose a reason for hiding this comment

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

This supposed to be here?

@marcoancona
Copy link

Why is this considered "not ready"? Something I can help with? We would also like to use Editor.js but we are stuck because we cannot merge edits from different users.

@mqtik
Copy link

mqtik commented Apr 18, 2021

I'd like to help too.

@mqtik
Copy link

mqtik commented Apr 18, 2021

Question:
Does this respect blocks already containing an ID?

@gohabereg
Copy link
Member

Closing due more relevant #1667

@gohabereg gohabereg closed this Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants