Sitecore / jss

Software development kit for JavaScript developers building web applications with Sitecore Experience Platform
https://jss.sitecore.com
Apache License 2.0
263 stars 275 forks source link

Link component change in href rendering behaviour without applicable unit test #1909

Open mg-aceik opened 2 months ago

mg-aceik commented 2 months ago

Describe the Bug

The change here https://github.com/Sitecore/jss/commit/b4309e8c7f8464c642e9dbe478b909f73df89ab9#diff-cd0f8f166dc72a9504e6cf7b0dfaaf409561f62d651b115c3e8185151162c7e1R36 in JSS 21.1.0 is a breaking change in that it needs a metadata property otherwise it will not render the link.

There isn't actually any unit test that is asserting this - e.g. you could have a field with a value and href, but if no metadata is set then it still returns null. I'm not sure if that was intentional or not and so I think a unit test should be added to assert that it was intentional.

To Reproduce

N/A

Expected Behavior

N/A

Possible Fix

N/A

Provide environment information

illiakovalenko commented 2 months ago

@mg-aceik Hey So this "if" statement is executed and returns null only in case if field is empty OR in case if value AND editable AND href AND metadata are not present. In this case if you have the field that contains value or href but doesn't have metadata it will not return null. Can you, please, elaborate more on this?

  if (
      !field ||
      (!(field as LinkFieldValue).editable &&
        !field.value &&
        !(field as LinkFieldValue).href &&
        !field.metadata)
    ) {
      return null;
    }
mg-aceik commented 2 months ago

Hi @illiakovalenko

Ok you're right I saw that my links that have a text value and no href are no longer rendering which was a change in behaviour from 22.0.0. I thought it was the change to the metadata field check but it's not, it must be something else, but I can't see what.

I've added unit tests here: https://github.com/mg-aceik/jss/pull/1/files If you run those tests I added against the 22.1.0 then they all pass. If you run those tests I added against the 22.0.0 then you get this failure:

should render nothing when field is present and href is not present:

  AssertionError: expected '<a href="undefined">ipsum</a>' to equal ''
  + expected - actual

  -<a href="undefined">ipsum</a>
art-alexeyenko commented 2 months ago

Hi @mg-aceik

I registered a bug for our backlog for this. Can you help us in prioritizing it, if you have a detailed repro scenario? Currently I see this happening if serialization went wrong, or if link raw values were modified directly. Are there situation when an editor can accidentally trigger this?

mg-aceik commented 2 months ago

This was just something my unit tests picked up for my components that were using links. I am ok with the change, I think, but just saw that there wasn't any unit test your side to cover the change, as I presume your team made this change to fix something but it has no unit test so it feels like it could be unintentional.

art-alexeyenko commented 2 months ago

@mg-aceik it is unintentional indeed, and more of a case that hasn't crossed our our field of view. We've added an item to backlog to address this, fortunately it's not a severe bug.