fix(shortcode): replace function should handle empty string callback return #16358
Conversation
packages/shortcode/src/index.js
Outdated
| @@ -102,7 +102,7 @@ export function replace( tag, text, callback ) { | |||
|
|
|||
| // Make sure to return any of the extra brackets if they weren't used to | |||
| // escape the shortcode. | |||
| return result ? left + result + right : match; | |||
| return typeof result === 'string' ? left + result + right : match; | |||
There was a problem hiding this comment.
Hi @dsifford thank you for your contribution. This change fixes the problem, but I noticed that if the callback returned a number previously the replace happened and with this change the replace is not happening. I added a test case for the scenario.
What if we use the condition result || result === ''?
There was a problem hiding this comment.
Up to you guys. I'd be more of a fan of making the return type more strict, but I'll concede to the majority. Non-strict return types can lead to some tricky bugs.
There was a problem hiding this comment.
I'd be curious what the usage looks like for someone who wants to opt out of the concatenation. In other words, if that usage should expect to provide a return value of undefined or null from the callback, couldn't we form the condition to target those values, which has the benefit of not breaking existing usage with values like numbers, while also type-specific to "empty" (skip) case.
Since I'd not really trust the result of + and potentially-numeric values (could add rather than concatenate?), I'd also not have too much of a problem with saying that the previous behavior was undocumented / unintended / buggy, calling this a "fix".
gziolo
added
[Type] Developer Documentation
|
@dsifford, can you refresh this PR so we could land it? |
There was a problem hiding this comment.
Based on:
Since I'd not really trust the result of + and potentially-numeric values (could add rather than concatenate?), I'd also not have too much of a problem with saying that the previous behavior was undocumented / unintended / buggy, calling this a "fix".
|
@gziolo Yes I can get this PR caught up with the main branch. I am forgetting the context now as to why I made this PR, but happy to make it mergeable if you all find this useful. |
|
If by refresh you didn't mean merge and instead you wanted a rebase let me know and I'll do that instead. |
|
Size Change: +5 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
Tests failing.. Shoot. Will look at this more later. |
|
Merge with Thank you for looking into it 👍 |
|
@gziolo It appears the only failing test is the one test case that was added after the fact in this same PR. Is that case still desired to be supported? (cc: @jorgefilipecosta) |
jorgefilipecosta
force-pushed
the
fix/shortcode-replace-empty-string
branch
from
2706007 to
d8a6d65
Compare
|
Thanks @jorgefilipecosta! 😄 |
jorgefilipecosta
force-pushed
the
fix/shortcode-replace-empty-string
branch
from
d8a6d65 to
9aea630
Compare
Description
Currently, the
replacefunction will not handle a callback that returns an empty string. This PR simply allows for that.How has this been tested?
Updated unit tests. All pass.
Screenshots
N/A
Types of changes
(depending on the interpreter) either Bug fix or Feature
Checklist: