hasClassName throws Error if component has property className which is not a string

[trmjoa]

A proposal to fix #452

I now see that there are PR's pending for related issues (#448). I'm not sure if the proposed fixes/code changes are overlapping.

If you feel its a viable approach, but want changes, please advise me on how to proceed and I will give it a try.

[aweary]

Thanks for the PR @Bannan! I think we will be going with #448 since it was opened first, but we appreciate the effort. We also don't want to simply return false when the className is an object, as that's essentially a silent failure, which is what we had before 2.3.0.

[aweary]

Can we just do the typeof check instead of all these checks on node? No matter what, if classes is not a string we should exit early since the next operation is a string operation.

[trmjoa]

I don't have the overview of the whole codebase, but I would consider it good if the method throws if its a native component and className is not a string. That should not be a silent error?
So;
node && typeof node.type === 'function' if that is true the node currently being processed is a custom component. If its not a custom component e.g a regular div, span etc, it will continue and have the same behavior as today, that is throw a error if you have passed something else then a string to className.

That's my take on it. What do you think? Your decision, of course :)

[trmjoa]

Updated the PR according to your comments. Didn't update the code inside hasClassName itself as I just wanted to clarify the behavior first. Whats your policy on patches. Do you want me to rebase and squash the commits into one?

[aweary]

@Bannan do you see this conflicting with #448 in any way?

[trmjoa]

Well... We're not using the mounted version of enzyme that much yet. But from looking at the code in MountedTraversal.js and the patch in #448 they both address issues around the class checking. The #448 patch just adds support for checking the classList property first if it exists, and if it doesn't and className is a object it uses the baseVal property if it exists.

The problem described in this issue (#453) doesn't exist in the mounted part of the code because in MountedTraversal.js a check for !isDOMComponent(inst) is executed. It will return false for custom components, I also do this in my patch so the behavior will be consistent with the version used in the mounted part.

The only "conflict" I see is that the patch in #448 should maybe be applied to the ShallowTraversal.js also, or at least ported. So that the behavior is consistent. Then both methods that check for classnames has the following behavior:

1. Check that this is a node representing a native html element(div,span, p etc); Otherwise return false
2. Check the classList property, if present check if the classname is in it and return the result.
3. If the instance doesn't have a classList property, check the className property. If it exists and is a object assign the objects baseVal property to the classes variable(this is to suppoert SVG elements).
4. Do the check as it is done today and return the result

[trmjoa]

@aweary Are you interested in getting this in, or should I close the issue?

[lencioni]

@aweary what do you think we should do with this PR?

[aweary]

I don't think we should throw if it's not a string, using an object with an overloaded toString method is perfectly valid. https://jsfiddle.net/tLs53LnL/

We should probably just coerce className to a string before calling replace.

[ljharb]

This looks worthwhile to get in in some form - either way it needs to be rebased, now that #448 is already merged.

[aweary]

@ljharb looks like we posted at the same time, ping just in case you missed my comment 🙂

[ljharb]

Coercing to string (with String() only) seems reasonable also.

[aweary]

@trmjoa do you want to update your PR to just coerce className to a string using String()?

[trmjoa]

I will rebase and update the PR over the weekend.

[ljharb]

The linked issue was closed with 3070850