-
-
Notifications
You must be signed in to change notification settings - Fork 7
Allow Mapping Block json() util to take arguments #548
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
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hi Gio, I thought we agreed that we didn't need this because we have existing Block functionality that's sufficient to do the same behaviour? |
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.
That's all good, code is clear and all tests passing. Nice!
Hello @lukestanley, I think you are talking about JSON.parse which we discussed in this PR #534. We agreed that even if we already have a block for parsing, it was a good idea to have it in mapping utility too so we merged that PR already. This is about making a value a string.
As you can see they are both using I personally find the name
But it will break a lot of Flows if I do that, so I just introduce a new utility and changed the documentation to explained better what json() utils does. I will now update the Issue Description so hopefully is clearer. |
@gsambrotta Thanks for disambiguation from jsonParse, I don't recall agreeing, but I see that you merged it to develop. I wasn't quite sure what the motivation for it was. This feels very similar but a different direction. Looking at your snippet and explanation, are you sure that is correct? Is this blocking any Flow work? I don't see an attached GitHub issue. Is there an agreed problem this Pull aimed at solving? I think we need to get better at having issues to scope the work we do. |
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.
It's not clear that this is a useful, well scoped change that solves a real problem.
Hey @lukestanley thanks for you feedback. Here are some answers:
When I was doing the PRs cleaning, I decided to merge it (as it was approved and green). To be completely honest, I have asked for your review several times on that PR, on github and on slack. Maybe you were busy, anyway it was nothing harmful, on the contrary i felt it is an improvement.
As I posted above the import rename stringify as asQueryString from 'qs'. In one function we are using
|
@lukestanley I have updated the code and the PR description as agreed in the meeting. |
I think a test for the existing functionality would be good, not just the new behaviour. |
Improve mapping
json()
utility function so that can take arguments.This is done in order to render a prettified JSON output as user needs to copy and paste it.
DONE:
json(data)
BEFORE:json(data, null, 2)
AFTER - using args: