Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking change proposal: Use exact string match by default #76

Closed
alexkrolick opened this issue May 4, 2018 · 10 comments · Fixed by #77
Closed

Breaking change proposal: Use exact string match by default #76

alexkrolick opened this issue May 4, 2018 · 10 comments · Fixed by #77
Labels
BREAKING CHANGE This change will require a major version bump enhancement New feature or request

Comments

@alexkrolick
Copy link
Collaborator

alexkrolick commented May 4, 2018

Describe the feature you'd like:

Make exact match the default instead of partial case-insensitive match. Regex would be recommended for those cases instead.

  • partial: /mystring/
  • case-insensitive partial: /mystring/i
  • prefix: /^mystring/
  • postfix: /mystring$/
  • case-insensitive full string: /^mystring$/i

One thing that would more involved to replicate with inline regexes is that the current implementation trims the string and replaces symbols with spaces:

- const normalizedText = textToMatch.trim().replace(/\s+/g, ' ')

See #74 (comment)

Suggested implementation:

I could see implementing this by exposing the final options argument to the getAllByAttribute helper to the public methods that use it so that exact: true could toggled.

https://github.com/kentcdodds/dom-testing-library/blob/master/src/queries.js#L73

Describe alternatives you've considered:

Leave the matcher alone

Teachability, Documentation, Adoption, Migration Strategy:

Show common regex patterns like the ones above^ in the docs.

Is this a breaking change?

Yes

@kentcdodds
Copy link
Member

I'm in favor of this change and I see no reason to go forward with a PR and push a major version bump. This probably should have been filed in dom-testing-library though because that's where the change will happen 😉 Though we'll need to do a major version bump of this lib as well to get that change.

@kentcdodds kentcdodds added enhancement New feature or request BREAKING CHANGE This change will require a major version bump labels May 4, 2018
@alexkrolick
Copy link
Collaborator Author

I'll add a link over there, too

@alexkrolick
Copy link
Collaborator Author

@alexkrolick
Copy link
Collaborator Author

Merged in https://github.com/kentcdodds/dom-testing-library/releases/tag/v2.0.0

  • update types
  • update examples
  • new release

@kentcdodds
Copy link
Member

Whoops, I probably shouldn't have just pushed an update to the version. I was being lazy. I'm pushing up a fix to the tests right now. We'll need an update to the types and examples. My bad :-/ Sorry about the laziness on my part.

@kentcdodds kentcdodds reopened this May 6, 2018
@maddijoyce
Copy link
Collaborator

I was thinking that the types for dom-testing-library should probably exist in that repo and then be imported from there. I can write the types there if you like?

@kentcdodds
Copy link
Member

That sounds great @maddijoyce, thanks!

@alexkrolick
Copy link
Collaborator Author

@maddijoyce sending a PR right now with what I think they are, you can finalize it.

@alexkrolick
Copy link
Collaborator Author

@maddijoyce #77

It's possible you'll want to remove a bunch of README stuff and point it to dom-testing-library instead

@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 3.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This change will require a major version bump enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants