Skip to content

Fix transformComponent to handle subtypes that do not return arrays#11

Merged
devongovett merged 2 commits intoottypes:masterfrom
thomsbg:master
Jun 27, 2015
Merged

Fix transformComponent to handle subtypes that do not return arrays#11
devongovett merged 2 commits intoottypes:masterfrom
thomsbg:master

Conversation

@thomsbg
Copy link
Copy Markdown
Contributor

@thomsbg thomsbg commented Jun 23, 2015

Needed to support rich-text for transform(). rich-text's transform function returns an object that does not pass a res.length > 0 test, and so is not appended correctly to dest.

This changes the logic to only perform the res.length > 0 test if res is an Array.

Some failing code that is now fixed:

var assert = require('assert');
var ot = require('ot-json0').type;
var a = [{ p: ['x'], t: 'rich-text', o: { ops: [{ insert: 'a' }] } }];
var b = [{ p: ['x'], t: 'rich-text', o: { ops: [{ insert: 'b' }] } }];
var t = [{ p: ['x'], t: 'rich-text', o: { ops: [{ retain: 1 }, { insert: 'a' }] } }];
ot.registerSubtype(require('rich-text').type);
assert.deepEqual(ot.transform(a, b, 'left'), t);

@devongovett
Copy link
Copy Markdown
Member

Can you add a test?

@thomsbg
Copy link
Copy Markdown
Contributor Author

thomsbg commented Jun 26, 2015

@devongovett I added a test by depending on the rich-text library.

@devongovett
Copy link
Copy Markdown
Member

We really should not depend on rich-text from here, even for the tests. You could just make a tiny subtype that tests just this particular behavior that you have changed.

@thomsbg
Copy link
Copy Markdown
Contributor Author

thomsbg commented Jun 26, 2015

Fixed to use a mock subtype

@devongovett
Copy link
Copy Markdown
Member

Looks good to me! Thanks.

devongovett added a commit that referenced this pull request Jun 27, 2015
Fix transformComponent to handle subtypes that do not return arrays
@devongovett devongovett merged commit 8974b41 into ottypes:master Jun 27, 2015
@devongovett
Copy link
Copy Markdown
Member

@josephg can you do an npm release at some point for this? Thanks!

@jvaill
Copy link
Copy Markdown

jvaill commented Feb 23, 2018

Would love a release of this as well. Thanks 😸

@josephg
Copy link
Copy Markdown
Member

josephg commented Feb 23, 2018

Hah its been awhile. Let me get on that

@josephg
Copy link
Copy Markdown
Member

josephg commented Feb 23, 2018

Published @1.1.0

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.

4 participants