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

Use picomatch instead of regex #369

Merged
merged 2 commits into from Feb 18, 2022
Merged

Use picomatch instead of regex #369

merged 2 commits into from Feb 18, 2022

Conversation

rockwotj
Copy link
Contributor

@rockwotj rockwotj commented Dec 29, 2021

Honestly should have done this in #354 but I didn't realize you could match relative path patterns using picomatch.

@agriffis
Copy link
Contributor

@agriffis agriffis commented Dec 29, 2021

Apart from my minor question about the test above, this looks great to me.

@agriffis
Copy link
Contributor

@agriffis agriffis commented Dec 29, 2021

As mentioned in #368, this will need to bump the major version of the plugin. Additionally we might want to consider changing the dep string in styled-components for the macro to ^1 || ^3 or along those lines.

@agriffis
Copy link
Contributor

@agriffis agriffis commented Jan 6, 2022

Rounding up some of the issues that will be fixed by this:

styled-components/styled-components#3635
styled-components/styled-components#3645
#350

@esetnik
Copy link

@esetnik esetnik commented Feb 18, 2022

Looks great to me. Thanks for the proper solution @rockwotj. What's left to get this merged?

@agriffis
Copy link
Contributor

@agriffis agriffis commented Feb 18, 2022

Just need to get @probablyup to notice it 👀

Copy link
Collaborator

@probablyup probablyup left a comment

Thanks!

@probablyup probablyup merged commit 996bc58 into styled-components:main Feb 18, 2022
5 checks passed
@agriffis
Copy link
Contributor

@agriffis agriffis commented Feb 18, 2022

Interesting, I thought you'd want to bump the major version for this. I suppose there probably aren't that many people relying on the regex matching, though, and this change will fix problems for a bunch of people (especially people using the macro), so maybe it's for the best.

@davidcostadev
Copy link

@davidcostadev davidcostadev commented Feb 18, 2022

OMG, this fixed my issue, thanks! I'm using it for displayNames on the react-scripts env <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants