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

Get rid of conversion API references from the mention attribute value #8370

Closed
oleq opened this issue Oct 28, 2020 · 3 comments · Fixed by #8718
Closed

Get rid of conversion API references from the mention attribute value #8370

oleq opened this issue Oct 28, 2020 · 3 comments · Fixed by #8718
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. intro Good first ticket. package:mention type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@oleq
Copy link
Member

oleq commented Oct 28, 2020

Provide a description of the task

  1. The attribute converter sets the attribute value using _toMentionAttribute() helper https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-mention/src/mentionediting.js#L52
  2. The helper gets the conversion API object during the conversion.
  3. The helper merges the conversion API object (data) with the attribute value https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-mention/src/mentionediting.js#L86-L102
  4. ...using Object.assign() in
    export function _addMentionAttributes( baseMentionData, data ) {
    .

And the result looks like this:

The attribute value is full of crazy references that have nothing to do with the content. This caused the ckeditor5-inspector to throw because of circular references (e.g. in writer) when inspecting a mention. Although the inspector is OK now (we'd hit this issue sooner or later, so the fix makes sense), the value of the mention attributes should be revisited because this asks for trouble.

@oleq oleq added type:task This issue reports a chore (non-production change) and other types of "todos". intro Good first ticket. package:mention domain:dx This issue reports a developer experience problem or possible improvement. squad:dx labels Oct 28, 2020
@oleq
Copy link
Member Author

oleq commented Oct 28, 2020

A hotfix https://github.com/ckeditor/ckeditor5/tree/hotfix/mention-conversion-api-update waiting for tests.

@jodator
Copy link
Contributor

jodator commented Oct 28, 2020

I'm not sure about the tests though - on one hand, we didn't catch that but on the other hand we probably could detect that extra attributes were set on the mention. :/

@oleq
Copy link
Member Author

oleq commented Oct 28, 2020

The missing assertion IMO is expect( Object.keys( attributeValue ) ).to.have.members( [ 'id', other props that make sense… ] )

@oleq oleq added this to the iteration 38 milestone Oct 28, 2020
@jodator jodator assigned jodator and unassigned jodator Oct 29, 2020
@jodator jodator modified the milestones: iteration 38, backlog Nov 23, 2020
@maxbarnas maxbarnas modified the milestones: backlog, iteration 39 Dec 22, 2020
@maxbarnas maxbarnas self-assigned this Dec 22, 2020
niegowski added a commit that referenced this issue Dec 28, 2020
…update

Other (mention): Conversion API reference is no longer passed down to attribute props. Closes #8370.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. intro Good first ticket. package:mention type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants