Skip to content
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

feat: Add JSONparseSafe helper #235

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

RomanKrasinskyi
Copy link
Contributor

What? Why?

We have the JSONParse helper which parse JSON and return an object:

{{#JSONparse jsonString}}
  {{name}}
{{/JSONparse}}

Helper works fine until non-json comes to parse. In this case it will break a page because it always expects syntactically correct JSON.

We faced with such case when parameter can be string or json and need to have a helper that can parse it safely.
According to required, we want to add a new JSONparseSafe helper:

{{#JSONparseSafe jsonString}}
  {{name}}
{{/JSONparseSafe}}

{{#JSONparseSafe jsonString}}
  {{name}}
{{else}}
  {{string}}
{{/JSONparseSafe}}

So, the helper try to parse passed parameter then

  • if succeeded - return an object
  • if failure - do nothing
  • if failure and we have else instruction - do else instruction

If you need more information please reach me

How was it tested?

Tested manually / Unit tests


cc @bigcommerce/storefront-team

@RomanKrasinskyi RomanKrasinskyi marked this pull request as ready for review December 21, 2022 11:58
@jairo-bc jairo-bc requested a review from bookernath December 21, 2022 13:28
@jairo-bc
Copy link
Contributor

cc @bigcommerce/dev-docs

@bookernath
Copy link
Contributor

Seems fine to me - @jairo-bc WDYT?

@RomanKrasinskyi it occurs to me from your description that perhaps the existing {{JSONparse}} helper should have a better failure mode, such as silently returning an empty string if the JSON fails to parse or only logging a warning. It's not great to have helpers that can completely break a page with bad input.

If we had such a thing, you could also do something like:

{{#if (JSONparse something) }}
  {{JSONparse something}}
{{#else}}
  <!-- do something else -->
{{/if}}

I have no problem with also creating this new helper, I just think we should fix the safety on the old one also. :)

@bc-traciporter
Copy link

bc-traciporter commented Jan 20, 2023

Created a ticket to implement these changes. See DEVDOCS-4750.

@RomanKrasinskyi
Copy link
Contributor Author

@bookernath
It's a good point to change behaviour of the existing helper. I thought to change {{JSONparse}} behaviour like in a new one, but we can break some thing. So we decided to create a new one.

In your suggestion we will parse json twice: once in condition and another one inside {{#if}} block. It look strange.

@RomanKrasinskyi RomanKrasinskyi merged commit f9edc1b into bigcommerce:master Feb 2, 2023
@RomanKrasinskyi RomanKrasinskyi deleted the LOCAL-1458 branch February 2, 2023 16:36
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

🎉 This PR is included in version 5.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants