@aws/xml-builder #7
Conversation
packages/xml-builder/lib/XmlNode.ts
Outdated
| */ | ||
| export class XmlNode { | ||
|
|
||
| public attributes: {[name: string]: any} = {}; |
There was a problem hiding this comment.
Why leave this public? It seems like you would always want users of this class to manipulate the attributes via the adder and remover methods below.
There was a problem hiding this comment.
Used public so tests wouldn't complain about ts compile errors, but I'll use the cast to any trick to get around it there.
packages/xml-builder/lib/XmlNode.ts
Outdated
|
|
||
| public attributes: {[name: string]: any} = {}; | ||
|
|
||
| constructor(public name: string, public children: XmlNode[] = []) {} |
There was a problem hiding this comment.
Ditto for these properties: why not make them private?
packages/xml-builder/lib/XmlNode.ts
Outdated
|
|
||
| constructor(public name: string, public children: XmlNode[] = []) {} | ||
|
|
||
| escapeElement(value: string): string { |
There was a problem hiding this comment.
This should probably also be private
packages/xml-builder/lib/XmlNode.ts
Outdated
| return value.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>'); | ||
| } | ||
|
|
||
| escapeAttribute(value: string): string { |
packages/xml-builder/lib/XmlNode.ts
Outdated
| let xmlText = `<${this.name}`; | ||
| // add attributes | ||
| const attributes = this.attributes; | ||
| for (let attributeName in attributes) { |
There was a problem hiding this comment.
for (let attributeName of Object.keys(attributes)) is more optimizable by V8 and will let you skip the has own property call on the next line.
There was a problem hiding this comment.
Sure, I can change that.
I did find https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#5-for-in suggesting that v8 can better optimize for..of.
https://groups.google.com/forum/#!topic/v8-users/hHetuuOaSmw indicates that since hasOwnProperty is being used, the speed difference is negligible.Mileage may vary based on the engine being used.
There was a problem hiding this comment.
Yeah, I'm not really concerned with the performance difference... for ... of lets you skip the hasOwnProperty boilerplate.
packages/xml-builder/lib/XmlNode.ts
Outdated
| xmlText += ` ${attributeName}="${this.escapeAttribute('' + attributes[attributeName])}"`; | ||
| } | ||
| // close the tag if there aren't any children | ||
| if (!hasChildren) { |
There was a problem hiding this comment.
Minor: you could write the rest of the function as single ternary expression.
packages/xml-builder/lib/XmlText.ts
Outdated
| /** | ||
| * Represents an XML text value. | ||
| */ | ||
| export class XmlText extends XmlNode { |
There was a problem hiding this comment.
I don't think inheritance is the right approach here, since XmlText instances will have inherited methods pertaining to children and attributes that will store data but not ultimately be used in the string output.
Maybe all you want to enforce is an interface with toString(): string defined? That would require escapeElement to be moved to a standalone function but would allow you to include anything other than null, undefined, or Object.create(null).
There was a problem hiding this comment.
That's fair, will update.
|
Updated PR to address feedback. |
packages/xml-builder/lib/XmlNode.ts
Outdated
| } else { | ||
| xmlText += `>${this.children.map(c => c.toString()).join('')}</${this.name}>`; | ||
| for (let attributeName of Object.keys(attributes)) { | ||
| xmlText += ` ${attributeName}="${escapeAttribute('' + attributes[attributeName])}"`; |
There was a problem hiding this comment.
Optional: You might want to add a test ensuring that XmlNode can handle undefined and null attributes without throwing an error. I could see the '' + getting removed during a refactor, especially if doing so caused no test failures.
|
I added a check so that undefined/null attributes will be ignored when XmlNode.toString() is called. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
2 participants
Adds an xml-builder package that contains just what our SDK needs to build XML documents. I tested this in the V2 SDK, and it passes all tests there as well.
/cc @jeskew