Create a DeletedText class. #55
Conversation
jogawebb
requested review from
harvestcore,
ins426,
luiisgallego and
thebassplayer1234
|
All components that use Text should now be able to use DelText, correct? If so, change it. |
There was a problem hiding this comment.
I have left some comments that I would like to have reviewed before merging. Perhaps the inheritance part is not possible, but it would be great to explore it.
Apart from that, before merging, I would also like to have a PR in FO updating it based on these changes, as these are really ‘breaking changes’ for FO, and we should have everything at the same time. :)
| @@ -1,4 +1,5 @@ | |||
| /** @jsx Docx.jsx */ | |||
| import { DeletedText } from '../../lib/components/track-changes/src/DeletedText.ts'; | |||
There was a problem hiding this comment.
In all example files, imports are being done using mod.ts.
| */ | ||
| export class Text extends Component<TextProps, TextChild> { | ||
| // Introduce a __brand property so that we cannot use Text and DeletedText interchangeably. | ||
| // TypeScript's structural typing would allow us to do so otherwise. |
There was a problem hiding this comment.
Outdated, but see my comment re: nominal typing and the Symbol() class below.
| import '../../document/src/NonBreakingHyphen.ts'; | ||
| import '../../document/src/Symbol.ts'; | ||
| import '../../document/src/Tab.ts'; | ||
|
|
There was a problem hiding this comment.
Why is this component not included in the Components > Document section?
There was a problem hiding this comment.
Well, as far as I'm aware, it only exists in the context of a tracked change, so placing it in track-changes/src felt like the better fit.
There was a problem hiding this comment.
I would still move it to Document, because it is just another type of element belonging to Document. Who knows if in the future we can use it for something more than track changes.
There was a problem hiding this comment.
As far as I know, it has to have an ancestor <w:del/>, which is why I think it's better suited to be with the tracked changes elements.
| /** | ||
| * A type describing the components accepted as children of {@link DeletedText}. | ||
| */ | ||
| export type DeletedTextChild = |
| * Deleted text is used within Word's track changes to indicate that text content in | ||
| * the document has been removed. `DeletedText` must have a parent {@link Deletion}. | ||
| */ | ||
| export class DeletedText extends Component<DeletedTextProps, DeletedTextChild> { |
There was a problem hiding this comment.
Is there no way to extend directly from Text so that only toNode() and fromNode() need to be overwritten?
There was a problem hiding this comment.
I'm pushing a change now that disallows using Text and DeletedText from being used interchangeably in all situations, but the consequence of forcing the two into their respective roles is that we can't use inheritance. They need at least one property to make their types unique, and DeletedText inheriting that from Text would mean you could use it accordingly.
There was a problem hiding this comment.
I think I understand what you mean. If you extend from Text, you cannot extend from Component with a unique type, and that causes some kind of conflict. Right?
There was a problem hiding this comment.
Exactly. We want DeletedText and Text to have unique type signatures so that they can't be used interchangeably when using our APIs. If they're identical or one just extends the other, then you could use the JSX or TS APIs to do make a structure that is invalid in Word.
| | CharSymbol | ||
| | Tab; | ||
|
|
||
| const __brand: unique symbol = Symbol(); |
There was a problem hiding this comment.
So, Symbol() is a built-in TS class that generates a unique symbol. But we also have a Symbol() class in docxml for representing certain text characters that are written symbols. That's why I import our Symbol as CharSymbol here.
I want to use TS's Symbol() to create a unique property for the Text and DeletedText classes so that they cannot have identical types. The name __brand is just a convention some people use when they create properties this way for nominal typing in TS.
…e to reflect that it's identical to TextChild.
| * Asserts whether or not a given XML node correlates with this component. | ||
| */ | ||
| static override matchesNode(node: Node): boolean { | ||
| return node.nodeName === 'w:r'; |
There was a problem hiding this comment.
Minor thing, ${QNS.w}r could be used here, right?
| it('serializes correctly', async () => { | ||
| expect(serialize(await deletedText.toNode([]))).toBe( | ||
| ` | ||
| <r xmlns="http://schemas.openxmlformats.org/wordprocessingml/2006/main"> |
There was a problem hiding this comment.
${NamespaceUri.w} can also be used here. Know that it is not in every other test, but...
| <r xmlns="http://schemas.openxmlformats.org/wordprocessingml/2006/main"> | ||
| <rPr><b/></rPr> | ||
| <delText xml:space="preserve">This text contains</delText> | ||
| <br xmlns:ns1="http://schemas.openxmlformats.org/wordprocessingml/2006/main" ns1:type="page"/> |
3 participants
This PR creates a
DeletedTextclass for use inside theDeletioncomponent. This removes the need to check the ancestry of aTextcomponent to determine if it should be created asdelTextortwhen we create a Word document.