Unify values returned in data.output during view-to-model conversion
#933
Conversation
|
Mmm... I'm unsure about this. Shouldn't we ensure that no method returns an array rather than post-convert this to DFs? |
|
Well, it depends. On one hand it feels safer. On the other hand you imply that view conversion always is to model. We struggled to not do this, but as we can see now: it had no point (it was a nice idea) and we already lost (markers has to be handled there and this already makes However since you can do: |
|
Okay, I have changed the solution, more info in the first post. |
| log.warn( 'view-conversion-dispatcher-item-not-converted: Item has not been converted (output is undefined).', input ); | ||
|
|
||
| data.output = null; | ||
| } else if ( !( data.output instanceof ModelNode || data.output instanceof ModelDocumentFragment ) ) { |
There was a problem hiding this comment.
I wanted to comment that it'd be better if we used duck typing with is() as it's safer. But I can see that we use a lot of instanceofs around our code still so we can change this at once in the future.
| @@ -180,6 +179,30 @@ export default class ViewConversionDispatcher { | |||
| this.fire( 'documentFragment', data, consumable, this.conversionApi ); | |||
| } | |||
|
|
|||
| // Handle incorrect `data.output`. | |||
| if ( data.output === undefined ) { | |||
There was a problem hiding this comment.
Is it really needed and correct? In what situations does it help?
There was a problem hiding this comment.
It seemed correct when I was working just on the dispatcher but "in real life" I can see that this is actually incorrect. There are situations where we expect that the view element will not be converted: for example when text is wrong with schema. On the other hand we have "default" converters which will take care of "non-intentionally not-converted elements". So yeah, this should be removed.
|
I fixed unneeded warning and fixed merge message. |
Tests: fixed some tests.
|
The build failed but tests are fine: |
| // 3. Extract children from document fragments to flatten results. | ||
| const convertedChildren = viewChildren | ||
| .map( ( viewChild ) => this._convertItem( viewChild, consumable, additionalData ) ) | ||
| .filter( ( converted ) => converted instanceof ModelNode || converted instanceof ModelDocumentFragment ) |
There was a problem hiding this comment.
This is like a black hole. Can't we just throw here? Or at least warn the developer. But I think that throwing is better because this isn't even a runtime error but simply a serious implementation bug. The same applies to _convertItem() – I feel that we could throw there as well.
There was a problem hiding this comment.
OK, _convertChildren() uses _convertItem() so the "wrong results" are not really wrong but just "non-convertable content". You can change the comment and it will be fine.
|
I commented on one issue. |
| // Flatten and remove nulls. | ||
| return convertedChildren.reduce( ( a, b ) => b ? a.concat( b ) : a, [] ); | ||
| // 1. Map those children to model. | ||
| // 2. Filter out wrong results. |
There was a problem hiding this comment.
Bad @scofalik cause bad comment!
| */ | ||
| log.warn( 'view-conversion-dispatcher-incorrect-result: Dropped incorrect conversion result.', [ input, data.output ] ); | ||
|
|
||
| data.output = null; |
|
|
||
| // Silences warnings that pop up in tests. Use when the test checks a specific functionality and we are not interested in those logs. | ||
| // No need to restore `log.warn` - it is done in `afterEach()`. | ||
| function silenceWarnings() { |
There was a problem hiding this comment.
I'm not 100% sure but I'm afraid that this may silence too many warnings as log may be defined just once at the beginning of this file. And if we ever switch to bundling all test files into one module (to improve Webpack's performance) this will silence all logs forever.
There was a problem hiding this comment.
Why? log.warn is restored in afterEach(). Anyway, I am fine with having those warnings as long as you are.
|
I pushed one more commit. You, again, start being PicKy... |
Reinmar
changed the title
data.output during view-to-model conversion
Suggested merge commit message (convention)
Fix: Unified values returned in
data.outputduring view-to-model conversion. See breaking changes. Closes ckeditor/ckeditor5#4056.BREAKING CHANGE:
ViewConversionDispatcher.convertnow always returnsmodel.DocumentFragment(which may be empty in various cases).conversionApi#convertItemcan pass onlymodel.Nodeormodel.DocumentFragmentornulltodata.output. Warning will be logged if different value was passed.conversionApi#convertChildrennow always returnsmodel.DocumentFragment.