[Fix] jsx-no-literals: trim whitespace for allowedStrings check
#2436
Conversation
lib/rules/jsx-no-literals.js
Outdated
| const isNoStrings = context.options[0] ? context.options[0].noStrings : false; | ||
| const allowedStrings = context.options[0] ? new Set(context.options[0].allowedStrings) : false; | ||
| const allowedStrings = context.options[0] ? | ||
| new Set((context.options[0].allowedStrings || []).map(trimIfString)) : |
There was a problem hiding this comment.
why the ternary? just making this unconditionally be a Set simplifies the check below
| } | ||
| } | ||
| `, | ||
| options: [{noStrings: true, allowedStrings: [' ']}] |
There was a problem hiding this comment.
could we add a few more tests, one with leading and trailing spaces on allowedStrings, and a different one with them on the rendered text?
There was a problem hiding this comment.
apologies, i'm not sure how to action this request. could you say it another way?
for context, we happen to have a pre-existing test with spaces in allowedStrings that would fail if allowedStrings were not trimmed. here are all the combinations of spaces and not spaces with test coverage:
| tested | spaces in allowedStrings |
spaces in rendered text |
|---|---|---|
| ✅ | ✅ | ✅ |
| ✅ | 🚫 | ✅ |
| ✅ | 🚫 | 🚫 |
| 🚫 | ✅ | 🚫 |
is that the test gap you'd like me to fix?
There was a problem hiding this comment.
I'd also like a test with noStrings false, and an allowedStrings value :-)
There was a problem hiding this comment.
✅ I've made noStrings: false explicit in the existing test case
|
possible to get this in a release soon? thanks for all the work you put into maintaining this plugin! 🙇 |
The
allowedStringsoption forjsx-no-literalsis checked against an untrimmed string containing potential spaces and newlines. This is at odds with the error message shown and substantially increases the difficulty of managing theallowedStringslist.I propose trimming strings before checking inclusion in
allowedStrings.related: