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

matchAll Polyfill #132

Closed
schonfeld opened this issue Aug 19, 2022 · 8 comments · Fixed by #133
Closed

matchAll Polyfill #132

schonfeld opened this issue Aug 19, 2022 · 8 comments · Fixed by #133
Labels
bug Something isn't working context-v1 Related to tailwind-merge v1

Comments

@schonfeld
Copy link

We've been seeing some client side error logs re .matchAll() calls evaluating to undefined. After a bit of digging, we realized that all of those exceptions originate on older-ish browsers. Nonetheless, this is an issue for us (and I suspect others, too). Specifically, the issue stems from this line:

for (const match of className.matchAll(/[:[\]]/g)) {

I'm curious if you've given thought to adding a matchAll polyfill, or using a different approach to matching?

@charkour
Copy link

For a workaround, you could consider using the npm package string.prototype.matchall to polyfill.

It provides a shim, so at the start of your app, I think you could call const matchAll = require('string.prototype.matchall'); matchAll.shim(); to polyfill.

Which browser are you using?

@schonfeld
Copy link
Author

Right -- I've impl'd that polyfill and it seems to work for us. But it took us an hour to realize this was the issue. Figured I'd post here to save others some time.

I'm able to reproduce this on Chrome 70.x on Mac. It's difficult to run multiple Chrome versions on the same machine, but you can download an old build of Chromium and easily run that side-by-side. Here's the version I'm using: version: 70.0.3538.124

Here's a sample stacktrace, although, the issue is rather straight forward:

image

@dcastil
Copy link
Owner

dcastil commented Aug 19, 2022

Just checked on caniuse and matchAll is available for 93.59% of global users. I'd say for a library like tailwind-merge that probably isn't enough, especially since I think I should be able to replace the functionality easily. I'll implement a fix for this.

@schonfeld out of curiosity, what browsers do you need to support? Do you have a defined policy for that?

@schonfeld
Copy link
Author

Just checked on caniuse and matchAll is available for 93.59% of global users. I'd say for a library like tailwind-merge that probably isn't enough, especially since I think I should be able to replace the functionality easily. I'll implement a fix for this.

@schonfeld out of curiosity, what browsers do you need to support? Do you have a defined policy for that?

I don't really a policy, per se. We typically try to not be bothered by older/out-dated browsers, but this issue seemed to affect a non-negligible portion of our daily users, somewhere in the 200 uniques per day... It seems to affect mostly mobile browsers. I suspect because those are usually tied to OS updates, and some folks are unable to upgrade on older devices...

image

@dcastil
Copy link
Owner

dcastil commented Aug 19, 2022

Okay, that makes sense. Thanks for the info!

@dcastil
Copy link
Owner

dcastil commented Aug 20, 2022

@schonfeld you can now <my-favorite-package-manager> add tailwind-merge@dev where matchAll isn't used anymore, so you should be able to remove your matchAll polyfill as well. A v1.6.0 will come in the coming days.

@github-actions
Copy link

This was addressed in release v1.6.0.

@schonfeld
Copy link
Author

Confirmed working on latest v1.6.0. Thanks so much, @dcastil !

@dcastil dcastil added the bug Something isn't working label Sep 30, 2022
@dcastil dcastil added the context-v1 Related to tailwind-merge v1 label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working context-v1 Related to tailwind-merge v1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants