-
Notifications
You must be signed in to change notification settings - Fork 1
Replace fast-diff within Delta #43
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
Conversation
|
Size Change: -1.53 kB (-0.08%) Total Size: 1.96 MB
ℹ️ View Unchanged
|
| // @ts-nocheck | ||
| /* eslint-env browser */ | ||
|
|
||
| import cloneDeep from 'lodash.clonedeep'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't ported over the tests as they are in a different framework, and I wasn't 100% convinced on converting them. Tbh, I'm hoping the license issue works out and we have no need for this PR.
That seems like a deal-breaker. |
Noting that this convo is continuing internally. |
|
Closing this PR as WordPress#72604 has been merged in with these changes. |
Description
Since the quill-delta library uses the Apache 2 license, while Gutenberg uses the GPL 2 license there may be a problem when we PR #41 into core. This change will swap the fast-diff library for the jsdiff library, as well as port the entire quill-delta package over so as to make the swapping easier.
The jsdiff has two major differences:
As we wait for legal's input on the licensing question, I'm leaving this in draft status as a backup plan.
The quill-delta test suite was really useful in seeing how seamless the swap could be. There was only one failure, related to the trivial semantic cleanup not existing. in that case, the end result was the same but fast-diff's result was what a human would do unlike the jsdiff one.
I'm going to keep testing it, to ensure I haven't missed any important difference.