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

add: support for old version of safari version 16 without break thing #14

Closed
wants to merge 2 commits into from

Conversation

hiepxanh
Copy link

@hiepxanh hiepxanh commented Dec 16, 2024

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and discussions and couldn’t find anything or linked relevant results below
  • I made sure the docs are up to date
  • I included tests (or that’s not needed)

Description of changes

as we discuss about "negative lookbehind" not working on old version safari in this issue: #10
I try to update it:

  1. fix windows issue while runing on fresh test
  2. add a check if current system support:
function transformGfmAutolinkLiterals(tree) {
  if (regexSupport.lookbehind) {
    modernAutolinkTransform(tree)
  } else {
    legacyAutolinkTransform(tree)
  }
}

the test is simple and running before we do anything else:

// Regex support detector
const regexSupport = {
  lookbehind: (() => {
    try {
      // Using regex literal instead of RegExp constructor
      ;/(?<=x)/.test('x')
      return true
    } catch {
      return false
    }
  })()
}

the result is fine:

The before change test:
before

The after change test:
after with node 22

I also try to install nodejs 16 but it not work, so I realize I cannot use many new syntax since nodejs 16 not support. Then I understand why you want to skip support.

I also try to build, to replace my current code to test on my ios, but the node_modules not found the autolink-literal since it is very deep link. I will try to test futher on ios 16.

I hope you can give me some thought about my idea. I don't mind if it useless and should not be merge, since I'm not have many experience. I would like to here what you think.

Thank you <3

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Dec 16, 2024

This comment has been minimized.

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

wooorm commented Jan 2, 2025

Appreciate you trying your hand at this!

CI is failing. That could be solved.

But importantly, in the other thread, I linked to and directly mentioned that “Different regex will be a performance problem for older browser”. This is important. I meant that as a blocker.

Your PR is essentially the same as for people who want to use old versions, to also use old versions. They can do that if they want/have to.

@hiepxanh
Copy link
Author

hiepxanh commented Jan 3, 2025

Happy new year 2025! <3 Thanks for the feedback!

Regarding the regex concern and the linked thread, I understand your point about performance on older browsers. That’s a valid blocker, I see your perspective. My intent was to provide a balance, but I’ll rethink the approach to align more closely with the broader goals of the project.

Thanks again for flagging these points! I understand now and we should keep your ways as it is the best as it could. Anyway, I love your plugin and all your ecosystem you build, really appreciate this, thank you so much

@hiepxanh hiepxanh closed this Jan 3, 2025

This comment has been minimized.

@wooorm
Copy link
Member

wooorm commented Jan 3, 2025

Happy new year Hiep! :)

@wooorm wooorm added the 🙅 no/wontfix This is not (enough of) an issue for this project label Jan 3, 2025
@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 Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

2 participants