sindresorhus / pupa

Simple micro templating
MIT License
362 stars 22 forks source link

Show undefined value as empty string #19

Closed jpiquot closed 3 years ago

jpiquot commented 3 years ago

I am using Pupa in my DevOps extension ChildTaskTemplate. A user reported to me that undefined values are interpolated with the 'undefined' string Issue 5. Is it possible to configure Pupa to use empty strings instead of 'undefined'?

sindresorhus commented 3 years ago

Yeah, there could be an option to use empty strings on undefined.

yuhr commented 3 years ago

It woud be better if it can notify users when undefined, by throwing an error instead of embedding empty strings silently. Can I file a PR?

jpiquot commented 3 years ago

I think that this behavior should be defined by setup as there are plenty of business cases where undefined values should not throw an error but just silently put an empty string.

yuhr commented 3 years ago

I see. Then the possible API would look like below:

const pupa = require('pupa')({
  emptyOnUndefined: true, // Default: `false`.
  throwOnUndefined: true, // Default: `false`. If `true`, it overrides `emptyOnUndefined` by its nature.
})

Or:

const pupa = require('pupa')({
  onInterpolate: (value, key, index) => value ?? "",
})

(I'd go for the onInterpolate way because it's generic and also covers multiple scenarios other than undefined)

Probably there might be some (edge-)cases to override some options at each templating, but it may go beyond pupa's original statement "Simple micro templating", so I feel a bit hesitate to add such API, which would be like this:

pupa(format, data, { ...someOverrideOptions })

What do you think about this idea?

jpiquot commented 3 years ago

Yes, the interpolate solution would be perfect.

sindresorhus commented 3 years ago

I think we should change the default behavior to throw on missing values. The common case is to have all the values, so should be loud by default in case of unexpected values.

Then we could introduce an option:

{
    missingValueBehavior: 'throw' | 'empty-string' | 'no-interpolate'
}

I'm happy to bikeshed the option name and the values.

I don't want to add onInterpolate as it seems we can solve this with a single easier option.

sindresorhus commented 3 years ago

I'm not 100% sold on falling back to empty string though. Can someone provide a short example of where falling back to an empty string makes sense? In such cases, I would argue, you should do the fallback in the code itself before passing the data to the template.

jpiquot commented 3 years ago

For example, you want to create child tasks from a DevOps workitem. You don't know anything about the custom fields created by the organization. How do you know what fields are used by the template? How can you dynamically add these empty fields to your data object if it's dynamic?

sindresorhus commented 3 years ago

I think I found a simpler way to handle this. Throw by default. And ability to not interpolate missing values:

{
    ignoreMissing: Boolean
}

And for the empty string case, we can do (as suggested earlier, with some small tweaks):

{
    transform: ({value}) => value ?? ''
}

Thoughts?

jpiquot commented 3 years ago

Yes thats nice.

yuhr commented 3 years ago
{
  transform: ({value}) => value ?? ''
}

If the placeholder text is also passed to the function, then it becomes capable of performing "ignore missing" as well, by returning the placeholder as is, for example:

{
  transform: ({value, placeholder}) => value ?? placeholder
  // where placeholder is a string including braces i.e. "{{key}}"
}
yuhr commented 3 years ago

And, "throw by default" behavior can be achieved by just having the default transform function be:

{
  transform: ({ value }) => {
    if (value !== undefined) return value
    else throw new Error()
  }
}
sindresorhus commented 3 years ago

If the placeholder text is also passed to the function, then it becomes capable of performing "ignore missing" as well, by returning the placeholder as is, for example:

True. But I still think it makes sense to have an option for this.

And, "throw by default" behavior can be achieved by just having the default transform function be:

No. I think throw by default should work no matter what. It would help catch problems if you accidentally return undefined in transform.

yuhr commented 3 years ago

But I still think it makes sense to have an option for this.

Wait, just to clarify, do you mean that you don't want to pass the placeholder text to transform?

I understand having a dedicated option to do some idiomatic stuff makes the code more readable when you just want it , but I'd always love to go with "single logic to all purpose", and I would expect transform to be an escape hatch to perform arbitrary operation, which needs some additional data to be available. That includes:

Though, if you feel this is not your design goal, just ignore me.

It would help catch problems if you accidentally return undefined in transform.

Hm, that's reasonable. Thanks pointing out.

sindresorhus commented 3 years ago

Wait, just to clarify, do you mean that you don't want to pass the placeholder text to transform?

I'm not against passing the placeholder text to transform.

and I would expect transform to be an escape hatch to perform arbitrary operation

I agree.

placeholder — with braces

Why does it need to include the braces?

yuhr commented 3 years ago

Why does it need to include the braces?

Because pupa's braces are single or double, and users (i.e. writers of this function) can't tell which one the original placeholder was. That is, without this info, transform can't conditionally fallback to "ignore" behavior correctly.

sindresorhus commented 3 years ago

But Pupa handles escaping, so transform doesn't need to care about that. Whether it's {} or {{}} shouldn't matter.

yuhr commented 3 years ago

Then how do you achieve "ignore if parseInt(value) is NaN, otherwise wrap it with angle brackets" for example? I thought I could write it like this:

transform: ({ value, placeholder }) => !Number.isNaN(parseInt(value)) ? `<${value}>` : placeholder

And I get:

pupa("{{0}} {{1}}", ["42", "undefined"]) // => "&lt;42&gt; {{1}}"

But if original braces wasn't provided, this is not possible, isn't it?

yuhr commented 3 years ago

Okay I got it, you mean I have to use ignoreMissing option in such cases like this, right?

ignoreMissing: true,
transform: ({ value }) => !Number.isNaN(parseInt(value)) ? `<${value}>` : undefined

Hm, it feels a bit weird solution for me though, I can still throw manually if needed, so it's okay...

sindresorhus commented 3 years ago

you mean I have to use ignoreMissing option in such cases like this, right?

Yes

sindresorhus commented 3 years ago

This is now "pull request" welcome.

yuhr commented 3 years ago

How about what timing it should accept options? Come to think of it, const pupa = require("pupa")({ ...options })-style API will introduce an unnecessary breaking change, so I'd suggest rather accepting at each templating, and if one wants to bake options in, then they can wrap it like:

const imago = (template, data) => pupa(template, data, { ...options })
sindresorhus commented 3 years ago

so I'd suggest rather accepting at each templating

Yes, that was the plan all along. I missed that your previous example did not do this.

It should be pupa(template, data, options?)