Skip to content

New 'negative lookbehind' regex breaks in IOS & Safari versions < 16.4 #10

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

Closed
4 tasks done
lllleonnnn opened this issue Aug 22, 2024 · 67 comments
Closed
4 tasks done
Labels
🙋 no/question This does not need any changes 👎 phase/no Post cannot or will not be acted on

Comments

@lllleonnnn
Copy link

Initial checklist

Affected packages and versions

2.0.1

Link to runnable example

No response

Steps to reproduce

Run latest 2.0.1 on any Safari version below 16.4

Expected behavior

Expected behavior is that no errors should be thrown

Actual behavior

When using this as part of rendering markdown in React apps, will throw SyntaxError: Invalid regular expression: invalid group specifier name and crash application.

Affected runtime and version

'browser'@2.0.1

Affected package manager and version

No response

Affected OS and version

iOS <= 16.3

Build and bundle tools

No response

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Aug 22, 2024
@ChristianMurphy
Copy link
Member

Welcome @nowfred! 👋
This project targets ES2022, older versions of the spec are not part of the support matrix.

Safari 16.4 and below do not support ES2022 out of the box.
You can transpile/polyfill this with babel, take a look at https://github.com/slevithan/babel-plugin-transform-regex

Related to remarkjs/react-markdown#800 and remarkjs/react-markdown#822

@ChristianMurphy ChristianMurphy closed this as not planned Won't fix, can't repro, duplicate, stale Aug 22, 2024
@ChristianMurphy ChristianMurphy added the 🙋 no/question This does not need any changes label Aug 22, 2024

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Aug 22, 2024
@lllleonnnn
Copy link
Author

Thank you @ChristianMurphy no worries - I personally appreciate finding issues like this when I'm debugging so wanted to leave for posterity. Appreciate your prompt reply and explanation.

@vktrl
Copy link

vktrl commented Oct 1, 2024

Welcome @nowfred! 👋 This project targets ES2022, older versions of the spec are not part of the support matrix.

Safari 16.4 and below do not support ES2022 out of the box. You can transpile/polyfill this with babel, take a look at https://github.com/slevithan/babel-plugin-transform-regex

Related to remarkjs/react-markdown#800 and remarkjs/react-markdown#822

@ChristianMurphy, please reconsider using a backwards-compatible regex or an option to specify one.

It can't be polyfilled and the babel transformer does not fix it.

Check the REPL and see that the pattern that's causing the problem is not being transformed: /(?<=^|\s|\p{P}|\p{S})([-.\w+]+)@([-\w]+(?:\.[-\w]+)+)/gu

For anyone else having this problem, I'm using pnpm to force compatible version for now:

// .pnpmfile.cjs

function readPackage(pkg, context) {
  if (pkg.name === 'mdast-util-gfm') {
    pkg.dependencies = {
      ...pkg.dependencies,
      'mdast-util-gfm-autolink-literal': '2.0.0',
    };
    context.log('mdast-util-gfm -> [email protected]');
  }

  return pkg;
}

module.exports = {
  hooks: {
    readPackage,
  },
};

@wooorm
Copy link
Member

wooorm commented Oct 1, 2024

Sad that that polyfill doesn’t seem to do lookbehinds.

“Backwards compatible” to what? The stone age? I don’t think it is reasonable to be compatible with ancient things.

There is of course some decent compatibility, which is outlined in the readme: https://github.com/syntax-tree/mdast-util-gfm-autolink-literal#compatibility. Sadly, Safari is more often than not, more recently, the place where things break first.

You can indeed use older versions if you want to.

@vktrl
Copy link

vktrl commented Oct 1, 2024

“Backwards compatible” to what? The stone age? I don’t think it is reasonable to be compatible with ancient things.

Safari 16.4 has only been out for 1.5 years. It's not from the stone age, and people are using it whether we like it or not.

I don't think it's unreasonable to ask for compatibility that goes back a little further than 2023. Especially if the solution is trivial and can't be polyfilled.

You can indeed use older versions if you want to.

I don't want to, but I'm forced to. This isn't something I don't have control over because it has to run client-side. In a perfect world people would keep their devices up to date, but alas.

@wooorm
Copy link
Member

wooorm commented Oct 1, 2024

Why do you think it’s trivial to solve?

You are of course right that the stone age was not 1.5 years ago.
However, you do understand that there is a point, somewhere, right?
There are also people that advocate the inverse of you: to not support old things.

When we cut releases, we drop support for old things. This is a breaking change that was communicated: https://github.com/syntax-tree/mdast-util-gfm-autolink-literal/releases.

@vktrl
Copy link

vktrl commented Oct 1, 2024

Why do you think it’s trivial to solve?

Because it's just one line in this commit.

You are of course right that the stone age was not 1.5 years ago. However, you do understand that there is a point, somewhere, right? There are also people that advocate the inverse of you: to not support old things.

There absolutely is a point somewhere, but is this really it? Breaking a recent browser with no workaround available because an improved regex pattern shaves off ~0.025ms when matching 5000 character strings?

@ChristianMurphy
Copy link
Member

There absolutely is a point somewhere, but is this really it?

It is

a recent browser

A browser version which is no longer supported by Apple

@ChristianMurphy
Copy link
Member

no workaround available

There are a number, you can hold on the older version of the package.
Try other transpiler/polyfills.
Or patch the old regex in if you want things to run slower https://github.com/ds300/patch-package#readme

@ChristianMurphy
Copy link
Member

My stance is pretty simple, if the world's richest company doesn't have time or money to support a version of their official and licensed product.
I don't have time to support it for free in my personal time.

@0xd8d
Copy link

0xd8d commented Oct 3, 2024

My stance is pretty simple, if the world's richest company doesn't have time or money to support a version of their official and licensed product. I don't have time to support it for free in my personal time.

Dismissing users on iOS < 16.4, which is still a fairly recent version, feels like an arrogant decision. It’s frustrating to see this issue brushed off when a simple regex tweak could easily prevent the crashes users are facing.

To make matters worse, this was a 2.0.0 to 2.0.1 update. A patch release should never introduce breaking changes. Dropping support for a significant chunk of users and telling developers to downgrade or deal with it isn’t just unreasonable, it’s irresponsible.

@wooorm
Copy link
Member

wooorm commented Oct 3, 2024

The breaking change happened in v2; nothing is brushed off, it was communicated; it’s not simple to change

@vktrl
Copy link

vktrl commented Oct 3, 2024

The breaking change happened in v2;

No, it happened in 2.0.1 with this commit as pointed out previously. 2.0.0 works fine.

it was communicated;

There's no mention of this in the readme or release notes.

it’s not simple to change

It is, but you've dug in and don't want to. It's just one line. I ran some benchmarks and the only tradeoff is shaving off fractions of milliseconds.

It's fine if you don't want to support Safari out of spite. It's your library and your choice. @ChristianMurphy was very straightforward about this and I got my answer.

I don't understand why you're gaslighting us over the facts though @wooorm?

@0xd8d
Copy link

0xd8d commented Oct 3, 2024

The breaking change happened in v2; nothing is brushed off, it was communicated; it’s not simple to change

It’s one thing to drop support for older browsers, but it’s another to ignore valid concerns when the fix is right in front of us. The fact that this issue popped up in 2.0.1, not in v2, shows this wasn’t a properly communicated breaking change. No one should have to comb through commits to figure out why a patch release suddenly breaks their app.

Saying the fix isn’t simple feels like an excuse when it’s a straightforward regex adjustment. We’re talking about preventing crashes for a significant number of users on Safari, and the performance hit is minimal at best.

@ChristianMurphy
Copy link
Member

I understand the frustration that many users have brought up in this thread, and I want to offer some clarity from my perspective.

First, it’s important to remember that the project's compatibility scope explicitly targets ES2022. The use of newer regex features like negative lookbehinds fits within that standard. Introducing features that align with the support matrix is not a breaking change and does not require special communication or break semantic versioning rules. Maintaining backward compatibility with older browsers, like Safari versions before 16.4, was never intended as a priority for this version.

The suggestions made, such as using tools like Babel or holding on to a prior version of the package, are not dismissive. They are practical options to meet the needs of projects requiring broader compatibility. I understand that polyfills and transpilers are not perfect solutions in every scenario, but they are part of the ecosystem's flexibility to bridge the gap between modern and legacy environments.

The comment about Apple no longer supporting Safari versions before 16.4 is significant. These versions are not receiving updates, which means users of those versions are already vulnerable to other issues. While I sympathize with the need to accommodate users restricted to older devices or browsers, it is also important for developers to focus efforts where there is active support.

Lastly, I want to make it clear that decisions about feature changes are not about spite or dismissiveness. They are choices made considering the larger technical roadmap and the constraints of developer time and maintenance complexity. Sometimes that means drawing a line where older environments are no longer supported.

The option to use an older version or a different configuration remains open for those needing it. Moving forward, embracing the latest standards is part of progressing in a landscape where technology evolves quickly. Thank you all for sharing your views. I respect the differing perspectives here, and I hope this helps explain the rationale behind the choices made for this project.

@0xd8d
Copy link

0xd8d commented Oct 3, 2024

I understand the frustration that many users have brought up in this thread, and I want to offer some clarity from my perspective.

First, it’s important to remember that the project's compatibility scope explicitly targets ES2022. The use of newer regex features like negative lookbehinds fits within that standard. Introducing features that align with the support matrix is not a breaking change and does not require special communication or break semantic versioning rules. Maintaining backward compatibility with older browsers, like Safari versions before 16.4, was never intended as a priority for this version.

The suggestions made, such as using tools like Babel or holding on to a prior version of the package, are not dismissive. They are practical options to meet the needs of projects requiring broader compatibility. I understand that polyfills and transpilers are not perfect solutions in every scenario, but they are part of the ecosystem's flexibility to bridge the gap between modern and legacy environments.

The comment about Apple no longer supporting Safari versions before 16.4 is significant. These versions are not receiving updates, which means users of those versions are already vulnerable to other issues. While I sympathize with the need to accommodate users restricted to older devices or browsers, it is also important for developers to focus efforts where there is active support.

Lastly, I want to make it clear that decisions about feature changes are not about spite or dismissiveness. They are choices made considering the larger technical roadmap and the constraints of developer time and maintenance complexity. Sometimes that means drawing a line where older environments are no longer supported.

The option to use an older version or a different configuration remains open for those needing it. Moving forward, embracing the latest standards is part of progressing in a landscape where technology evolves quickly. Thank you all for sharing your views. I respect the differing perspectives here, and I hope this helps explain the rationale behind the choices made for this project.

Yes, this project targets ES2022, but the reality is that this change broke things for a huge number of users still on Safari < 16.4. You can argue all day about technicalities and semantic versioning, but when the end result is crashes and broken apps, it’s a breaking change. Period. And in a patch release like 2.0.1, which developers expect to be stable, this kind of oversight isn’t just frustrating, it’s a major failure in communication and responsibility.

Telling developers to either roll back to an older version or use Babel isn’t a “solution.” It’s a cop-out. Not everyone has the flexibility to introduce complex build setups just to accommodate a change that could easily be fixed with a single line tweak. Why should the burden be on us to patch a simple regex issue that your team introduced in the first place?

And as for dismissing Safari < 16.4 because Apple doesn’t support it anymore, that’s a weak excuse. Just because Apple moves on doesn’t mean the rest of the world does, and pretending those users don’t exist doesn’t magically make the problem go away. We’re not talking about browsers from a decade ago, we’re talking about versions still actively used by millions. Writing them off like they don’t matter is irresponsible.

Your stance on “embracing the latest standards” feels more like stubbornness than progress. This isn’t about evolving technology, it’s about prioritizing the community that depends on your library. Developers trusted your package, and now they’re dealing with unnecessary crashes because you refuse to address an easily solvable issue.

The fix is trivial, the impact is massive, and the refusal to handle this properly is a clear signal that the user base isn’t being considered. If the goal is to alienate developers and break apps, you’re doing a great job. If not, this really needs to be addressed, not dismissed with empty justifications.

@ChristianMurphy
Copy link
Member

I appreciate your perspective and the time you've taken to lay out your concerns. Let me respond in kind, acknowledging both the technical realities and the broader considerations at play.

First, this project, like many others targeting modern JavaScript standards, follows a well-defined compatibility matrix. It’s true that users of Safari < 16.4 have experienced issues, but the decision to target ES2022 is not arbitrary. It's based on the desire to push forward and leverage features that improve performance, maintainability, and long-term viability of codebases. This benefits the broader community, millions of users who appreciate faster load times and more efficient code execution.

Regarding the patch update, it's worth remembering that "stable" doesn't always mean "unchanged forever." Stability in software is as much about keeping up with the standards as it is about consistency. We chose to implement a feature that aligns with ES2022, which is where the broader JavaScript ecosystem is headed. A move towards newer standards is essential for the health of the ecosystem. Introducing transpilers or polyfills to accommodate those browsers is not an unreasonable ask. It's a well-established practice in the development community. Adopters need to adapt to the standards or use the tools available, instead of expecting those moving forward to hold back for a few who haven’t kept pace.

You mentioned that the solution is trivial. But let’s be clear, what might look like a trivial regex tweak isn’t just about altering a line of code. It's about the implications for performance and maintainability for the vast majority of users. The current implementation provides meaningful gains, even if those gains seem small to you. However, when multiplied across millions of uses, these incremental improvements can significantly impact the efficiency of applications. Asking us to revert a valuable enhancement to accommodate older, unsupported environments is the very thing that holds back progress. We can't let decisions for the many be dictated by the few who are unwilling or unable to adapt their tools.

On the topic of maintaining backward compatibility, I have to say it's a bit disappointing to see developers who have never contributed to this project or even to open source in general become so vocal about demanding changes. Open source is built on collaboration and contribution. If this library truly matters to you, there are avenues for engaging constructively, whether by contributing code, proposing meaningful improvements, or even adding documentation to help others navigate these changes. Instead, I see repeated calls to "fix this trivial issue" without an offer to be part of the solution. Perhaps it is easier to criticize than to contribute.

As to those personal remarks about "stubbornness" or implying this stance is irresponsible, I would suggest considering the broader picture. The community relies on maintainers, many of whom do this work in their personal time, not just to maintain stability but also to drive innovation. If the richest companies out there have moved on from supporting these older environments, it's not a trivial ask for open-source maintainers to spend their limited time and resources filling in those gaps.

Lastly, I genuinely want what's best for developers using this library. You always have options, whether that means staying on an older version, using a transpiler, or even forking the library and making the changes yourself. Those are not dismissive responses. They are practical realities in the world of software development. The commitment to progress means making choices that sometimes leave legacy systems behind, but ultimately pave the way for a more robust, capable, and efficient ecosystem.

The message is clear: progress comes with change, and embracing that is a responsibility we all share.

@vktrl
Copy link

vktrl commented Oct 4, 2024

We can't let decisions for the many be dictated by the few who are unwilling or unable to adapt their tools.

If the richest companies out there have moved on from supporting these older environments, it's not a trivial ask for open-source maintainers to spend their limited time and resources filling in those gaps.

I agree with this sentiment when it comes to dev tooling, but we can't push updates to client devices. I did a quick check and it looks like about 5% of my users on iOS are affected by this.

It's weird to frame it like it's somehow supporting Apple and their shitty browser vs the actual real-world users who are using older devices for one reason or another. I try to use the latest standards possible, all in on ESM, etc, but I choose 5% of end-users being able to use my software over "embracing progress and change" every time.

First, this project, like many others targeting modern JavaScript standards, follows a well-defined compatibility matrix. It’s true that users of Safari < 16.4 have experienced issues, but the decision to target ES2022 is not arbitrary.

This has nothing to do with ES2022. It's a regex feature that was introduced in Node 8 and in Chrome in 2017. The real issue here is Safari being the modern IE and giving us another edge case to handle.

All solutions have tradeoffs:

a. Revert the change in the library and trade a little performance for compatibility.
b. Add an escape hatch to allow usage of the old regex for compatibility, increasing code complexity.
c. Have library consumers deal with it. Tradeoff being developers having to add more unmaintainable jank to their tooling until the market share drops low enough to warrant kicking out the last few who haven't upgraded.

Again - I'm fine with your decision, I have a fix in place. I understand that they are MY users not yours, and you have no obligation support them. What really annoys me is not acknowledging and misrepresenting what the issue is and what can be done about it. "Fuck Safari, you deal with it" is much less insulting to me, personally.

@wooorm
Copy link
Member

wooorm commented Jan 7, 2025

you should read the compatibility section in the readme and you should read the license. You should also pay the people that build your infrastructure. Finally, you should use lock files and test your apps.

@skyriverbend
Copy link

I did read the compatibility section. There's no browser matrix or anything indicating incompatibility with a recent version of safari:

I've locked to 2.0.0 now. But you have to admit this is a breaking change.

Also I've sponsored open source contributors for thousands of dollars. I'm genuinely grateful for all the work you've done. I'm just trying to raise a voice here, because I'm sure this is going to continue to cause bugs for thousands more users if you don't actually take it seriously.

@wooorm
Copy link
Member

wooorm commented Jan 7, 2025

  • the inclusion of Node and absence of iOS in a compat matrix gives you enough info
  • no, I do not think this is a semver breaking change, iOS is explicitly not covered in our compatibility matrix, but Node is: there will always be engines that are not tested by us, that do not support things that we use. Safari will keep on breaking and breaking because they do not invest in the web, and do not implement features that landed 8 years ago in v8
  • it is sad when things break and we often prevent things breaking, but we cannot reasonably prevent this break, because it is a trade off between things, compat + performance
  • while you feel unheard you are not, while you feel treated unseriously, you are not

@neil-morrison44
Copy link

Since your compatibility section doesn’t mention anything other than node, iOS is implicitly not covered by it, if you want to explicitly not cover it (or, presumably any other web browser) you need to actually mention it.

Did the PR to patch this go anywhere?

@skyriverbend
Copy link

Literally as you sit here defending your actions, an open source app with 20k stars and hundreds of thousands of users just reported having their app randomly break on users.

danny-avila/LibreChat#5204

How can you justify this? Especially considering the comments in this thread indicate that the regex provided only a marginal performance improvement. You're cutting out 6% of users on the internet with this tiny regex line that you refuse to change.

Continuing to ignore this issue is causing active harm.

@wooorm
Copy link
Member

wooorm commented Jan 7, 2025

if you want to explicitly not cover it [...] you need to actually mention it.

That’s not what I want.

marginal

6%

These numbers are incorrect. Please do not misrepresent.

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jan 7, 2025

Since your compatibility section doesn’t mention anything other than node, iOS is implicitly not covered by it, if you want to explicitly not cover it (or, presumably any other web browser) you need to actually mention it.

@neil-morrison44 that isn't how support matrices work.
Everything listed is explicitly supported, everything not listed is explicitly not supported.

Did the PR to patch this go anywhere?

You can read for yourself #14

this thread indicate that the regex provided only a marginal performance improvement

@skyriverbend It is significant

How can you justify this?

The entitlement and narcissism in your comments is stunning.
Different projects have different support matrices, the world does not revolve around you.
There is nothing to justify, older browsers are supported by older versions of the library.
You can continue to use the previous version, or you can encourage your users to upgrade.

You're cutting out 6% of users on the internet with this tiny regex line that you refuse to change.

No, that number is both inaccurately over represented, and for the actual small fraction of users, they can use the old version until they are ready to upgrade.
This backwards IE style mindset is what causes broken slow packages, Marvin Hagemeister did a great series in performance https://marvinh.dev/blog/speeding-up-javascript-ecosystem-part-6/
Polyfills and excessive de-optimization for legacy browser support can be significant bottlenecks, hurting everyone on the internet, not a small fraction.

@neil-morrison44
Copy link

Assuming that https://github.com/remarkjs/remark-gfm?tab=readme-ov-file#compatibility is the extent of where you talk about compatibility it isn't a matrix. You are explicitly supporting maintained versions of node, and you could read into that that you're implicitly not supporting anything else.

While I don't endorse the tone / content of the other recent commenter, rejecting a PR which would have resolved this issue for you and users of your library baffles me. The pragmatic approach would surely to have been to accept it until usage of that old version of Safari drops off completely and you can remove it without risk of getting all these annoying github comments from people who find this issue after having to dig through their compiled JS to find the error that users of old versions of Safari were reporting just like myself and LibreChat did.

I don't expect you to do work for free, but I would expect some degree of good stewardship of your open source project.

@ChristianMurphy
Copy link
Member

is the extent of where you talk about compatibility it isn't a matrix

It is a matrix, we support LTS versions of Node https://nodejs.org/en/about/previous-releases

You are explicitly supporting maintained versions of node, and you could read into that that you're implicitly not supporting anything else.

Yes, that is how matrices work, anything not explicitly listed, is explicitly not included.
I would like to think that the project does not need to explain what a support matrix is in each of the 600 repositories to avoid pedantic commenters.

The pragmatic approach would surely to have been to accept it until usage of that old version of Safari drops off completely

Pinning the version is both pragmatic and principled.
Safari 16.4 already has fallen off, you can of course choose to support it, and the older version is there to allow you to do so.
Don't force the rest of the internet to have either a bloated package or bad performance to support your legacy matrix, that would be bad open source stewardship.

@thatgriffith
Copy link

I used resolutions to lock the version to 2.0.0.

{
 "resolutions": {
    "mdast-util-gfm-autolink-literal": "2.0.0"
  }
}

Used your solution to solve my problem.

In our case, we were using remark-gfm in a React project. We had upgraded the projects a few months ago and never thought about testing it on iOS16.4 and below.

@wy1009

This comment has been minimized.

@consistent-k
Copy link

@thatgriffith Would it be possible to wrap the regex execution inside a try-catch block?
https://github.com/syntax-tree/mdast-util-gfm-autolink-literal/blob/main/lib/index.js#L131C3-L138C4

@thatgriffith
Copy link

@thatgriffith Would it be possible to wrap the regex execution inside a try-catch block? https://github.com/syntax-tree/mdast-util-gfm-autolink-literal/blob/main/lib/index.js#L131C3-L138C4

Not sure. You could give it a try. Otherwise I would just suggest to lock it to version 2.0.0

@MatthewPattell
Copy link

Don't forget about patch-package if you don't want to downgrade version. Just replace regexp and apply your patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 no/question This does not need any changes 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests