fix(startup error): update @testing-library/dom to update pretty-format

[johncrim]

API change while updating to new @testing-library/dom

fixes: #519

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https:/ngneat/spectator/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x ] Bugfix
[x ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #519

What is the new behavior?

Karma tests run without failure.

And: Matchers can now be numbers - a number matcher just matches number text, so the behavior is the same as if the matcher were numberMatcher.toString(). Because the change to the Matcher signature comes along with the dependency update required by this fix, the bug fix and exciting new feature come together.

Does this PR introduce a breaking change?

[ x ] Yes
[] No

The breaking change will occur due to the change to MatcherFunction in @testing-library/dom - the MatcherFunction now passes in an Element, where it previously was HTMLElement - so users will have to test/cast to HTMLElement if they need HTMLElement-specific properties in their matcher function.

It's unfortunate that this change was added in a minor version of @testing-library/dom. Since spectator depends on this external interface that changed, there's no way (that I can find) to fix this major issue without taking the API change.

Other information

The @testing-library/dom API changed a little, so I had to update byTextContent() to fix some typing errors - no issue there, the new API is less strict, so there should be no change to the spectator API.

The issue I found is that type Matcher can be a number, and if it is a number it would have thrown with the previous code. I added a check to prevent that error (also needed to compile), but I'm not sure what the correct behavior is for a number matcher, so I'm throwing an exception. If you'd like I can add tests to verify that an exception is thrown, but it's also possible that different behavior is expected if a number matcher is passed in. Please clarify what the correct behavior is.

edit: I removed the "throw error on number Matcher" and added support for number matchers in byTextContent().

[NetanelBasal]

@benelliott what do you think?

[benelliott]

Will take a look

[benelliott]

@johncrim These changes look good to me, but while you're there, do you think it would be worth adding support for the number-type matcher as per testing-library/dom-testing-library@ec1b642 rather than erroring out?

If we keep the code similar to dom-testing-library it would be as simple as adding || typeof matcher === 'number' on line 36 and .toString() on line 42.

I was trying to decide if this would be a breaking change since we are exposing the type of Matcher via our selector APIs. Technically it would since that type has been widened from HTMLElement to Element, but it's worth noting that this breaking change was actually introduced in a minor release of dom-testing-library (v7.29.0).

[johnc-ftl]

Thanks @benelliott - that sounds good. I wasn't aware of how the number matcher works, so I'll dig into that/follow your recommendations.

RE possible breaking change, in @testing-library/dom matches.d.ts it's interesting that MatcherFunction was widened to:

export type MatcherFunction = (
  content: string,
  element: Element | null,
) => boolean

But Match.node is still HTMLElement:

export type Match = (
  textToMatch: string,
  node: HTMLElement | null,
  matcher: Matcher,
  options?: MatcherOptions,
) => boolean

I haven't dug in enough to determine if this node type is incorrect (would a node be returned of type Element but not HTMLElement if indicated by the matcher)? If the node type is correct we should be fine - type widening on inputs, but no changes to output shouldn't break anyone. Anyhow this is probably well outside this issue...

[benelliott]

Actually due to covariance/contravariance I think the widening of the parameter type of the MatcherFunction could break people - e.g. say you have an admittedly weird matcher function (_, x: HTMLElement) => x.title === 'foo', this would be a compilation error now that x is Element as title is defined on the HTMLElement subclass.

[johnc-ftl]

@benelliott - agreed, I was thinking about it backwards - thanks.

I think we're in a compatibility pickle then, since existing users have a dependency on @testing-library/dom Matcher and MatcherFunction, and those signatures have changed. If we update @testing-library/dom anything built against those signatures can break (in addition to adding the number matcher, which is non breaking, but should be tested). If we had a spectator-internal interface then we could manage this change.

So, the question is whether to:

1. Update pretty-format only, and not take any changes to @testing-library/dom. This is the safest change from a compatibility standpoint, but means we won't be able to use anything in newer versions of @testing-library/dom without taking the same breaking change.
2. Update @testing-library/dom and document the API change in release notes (is there a way to do that?) and risk a compatibility change.

I'm thinking the best thing to do right now is to kick the can down the road, and update pretty-format only - we need that fix urgently. The @testing-library/dom update can be done with a major version update (if anyone expresses a need for it).

Revised PR coming right up...

[johncrim]

Crap - I can't figure out a way to update transitive dependencies in a way that is reliable across generally used versions of yarn and npm. Yarn has resolutions and npm has overrides documented, but they don't seem to be working reliably. I need to update the version in @testing-library/dom's node_modules dir.

So, updating @testing-library/dom is the only fix I can find, which comes with the potentially breaking API change. IMO this issue is severe enough (I'm blocked on it) that the API change is worth it.

I will add some tests for the number matcher.

[johncrim]

I removed the "throw error on number Matcher" and added support for number matchers in byTextContent().

Presumably the other methods that take matchers can also take number matchers - this should be handled automatically by @testing-library/dom. I didn't add tests for all of them, partly b/c the scope of this has grown a lot from my initial plans - just trying to fix a transitive dependency issue.

I think the breaking change to MatcherFunction API is not ideal (I updated the PR info above to describe it), but I think it's necessary to update @testing-libary/dom, which is the only fix I can find for this severe/blocking issue. I don't know that it belongs in the docs, but it should be in release notes if available.

[NetanelBasal]

I will merge after @benelliott approval. Thanks.

[benelliott]

Thanks @johncrim for the effort with this - once you remove that outdated comment I think this looks good!

Not sure if @NetanelBasal has any thoughts on the minor breaking change.

[benelliott]

Outdated comment

[NetanelBasal]

@benelliott I'm not familiar with this library.

[NetanelBasal]

We could bump to a major release if you guys think this is a breaking change. @benelliott @johncrim

[benelliott]

It is technically a breaking change due to the change to MatcherFunction described above, but it's a fairly small breaking change at that.

[NetanelBasal]

If it'll break someone app it should be a major release

[johncrim]

I agree, though the change was breaking it was not a major release for @testing-library/dom. I wish they hadn't made that change.

As long as it's not big deal to you, I think the major release is correct. This change does make a major version change to the @testing-library/dom dependency, which is part of the spectator API.

[johncrim]

Thanks @benelliott and @NetanelBasal - comment removed.

[benelliott]

Thanks!

[NetanelBasal]

@johncrim, Can you please update the commit message to include the breaking change so that the standard version package understands it?

[johncrim]

@NetanelBasal I rebased to fix the commit message - also improved the wording of the feat(number matcher) commit, and squashed the last commit. So, same file changes, hopefully improved commit wording.