Closed jeanlauliac closed 10 years ago
@jeanlauliac thanks a lot! Generally PR is good, but it brokes the back-compatibility of the public API.
@Flackus what are you think? Looks like we must accept this PR and bump major version number.
@jeanlauliac @kaero Hi :) Though I understand the perfectionism of such an issue, I would say that it's not worth breaking backwards compatibility. Sometimes naming is just naming. Nonetheless, if we ever break compatibility, we should definitely close this issue.
Can we use both fields? Later we can just remove the wrong one.
@miripiruni man, you're right! :+1:
@jeanlauliac can you slightly change your PR and add the childs
property as the reference to the children
array?
Yep I can try this, and we can make it non-enumerable so that is doesn't leak when converting to JSON. Note, though:
childs
is nowhere referenced in the README.md
, so it only breaks a non-documented feature (this should probably be added -- will make another PR maybe);< 1.0.0
, and "If you're worrying a lot about backwards compatibility, you should probably already be 1.0.0." says the semver ref :wink: even though I agree such change would require the minor number increase.we can make it non-enumerable so that is doesn't leak when converting to JSON
I prefer to fix NodeSet#toJSON
method to include only children
to output. Using Object.defineProperty
slow down XML tree building for a times. I was tried to use it in initial development and drop it finally before release due to the lack of desired performance.
"If you're worrying a lot about backwards compatibility, you should probably already be 1.0.0."
Turned out, that we have a bunch of the projects in production and in the active development, which already use xamel. So the cost of breaking back-compatibility without heavyweight reasons is to high for us – it's the only reason to keep it :crying_cat_face:
Do you have a time to finish PR? If not, we'll fetch your changes and finish them in some days. I'm asking to avoid doing concurrent work for the module..
Oh, sorry guys, i'm stupid :tired_face:
childs
property is not a part of the public API, so we can rename it now.
Well yes, that's why I was saying "childs is nowhere referenced in the README.md" :P
Thanks for merging!
That'd be nice and simple but unfortunately 'childs' is not valid in english :P
This change aims to correct all the references to the proper word, 'children'. I just made a whole-directory search-and-replace. Tests pass.