-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Add gatsby-transformer-xml #1465
Conversation
Deploy preview ready! Built with commit 1848894ddd29774dcb557361c152f35a822f972a |
Deploy preview ready! Built with commit 1848894ddd29774dcb557361c152f35a822f972a |
Deploy preview ready! Built with commit 34e1ca6 |
Deploy preview ready! Built with commit 1848894ddd29774dcb557361c152f35a822f972a |
Deploy preview ready! Built with commit ad2f8ccc98a24ea02d22eb38b0c5625070e831c8 |
Deploy preview ready! Built with commit 34e1ca6 |
Deploy preview failed. Built with commit ad2f8ccc98a24ea02d22eb38b0c5625070e831c8 https://app.netlify.com/sites/image-processing/deploys/5964e005a700c4202e22c642 |
Deploy preview failed. Built with commit 42af351e721090974bfc6e2ed8fb58130cfe2144 https://app.netlify.com/sites/using-glamor/deploys/5964be85cf321c20b7caf54b |
Deploy preview failed. Built with commit 42af351e721090974bfc6e2ed8fb58130cfe2144 https://app.netlify.com/sites/using-contentful/deploys/5964be85cf321c20b7caf54f |
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.
So... I'm having doubts about even doing an XML transformer. Not that it wouldn't be valuable but XML is used in such complex & unexpected ways that I'm not sure we could build a generic XML transformer that'd be useful.
Perhaps it'd be better to just have a nice docs page on building your own XML transformer to meet the needs of a specific project?
"id": "bk101", | ||
"internal": Object { | ||
"contentDigest": "446732570969e52599dded1ba4941b65", | ||
"type": "UndefinedXml", |
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.
"Undefined" looks like this needs fixed
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.
@KyleAMathews I just found the same issue in the gatsby-transform-json
should it be node.internal.name
instead of node.name
?
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.
Oh hmmm... ok I guess perhaps this and the JSON test are kinda correct. If the node you're building from is a File node then node.name
would be the file name. So for the tests we should just set a name on the fake test nodes so the snapshots don't look weird.
Could you do that here + to the JSON transformer test?
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.
Will do
@Khaledgarbaya thoughts on my comment above? |
Hey, sorry I didn't respond i am away from computers for some time. The
library I'm using should cover pretty much everything, i see your point for
the advanced usage but still having the transform plugin is nice to have
On Tue 25. Jul 2017 at 23:16, Kyle Mathews ***@***.***> wrote:
@Khaledgarbaya <https://github.com/khaledgarbaya> thoughts on my comment
above?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1465 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABGj_deBcywJoWqdqbCtCmikwfXuarQsks5sRmk8gaJpZM4OUIbQ>
.
--
--------------------
Best Regards,
*Khaled*
|
Ok! I agree, yeah, something is better than nothing and if people with advanced or unusual needs find this transformer isn't meeting their needs, it'd be easy to fork and customize so better to have something than nothing. There was what looks like one bug that I left a comment on. Please fix that and let's get this released! |
@KyleAMathews Done, and I started an other PR to fix the JSON transform tests |
Thanks!! This is a cool transformer :-) @Khaledgarbaya btw, the README said it transforms "JXML" which I assume was a typo? I pushed a change before merging but return it if that's what you meant. |
Summary
This PR adds the logic for
gatsby-transform-xml
packageTODO