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

fix: improve InclusiveDescendant performance #16

Closed
wants to merge 1 commit into from
Closed

Conversation

JounQin
Copy link
Member

@JounQin JounQin commented Jan 16, 2025

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

microsoft/TypeScript#51188

Restrict the depth to avoid Type instantiation is excessively deep and possibly infinite

Note

The PR target is v5

@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 Jan 16, 2025
@wooorm
Copy link
Member

wooorm commented Jan 16, 2025

the issue you reference is old, and I already fixed it in 766107b?

@wooorm
Copy link
Member

wooorm commented Jan 16, 2025

Why stay on v5? Why not use more modern utilities?

@JounQin
Copy link
Member Author

JounQin commented Jan 16, 2025

@wooorm I'm using https://github.com/web-infra-dev/rspress

See also web-infra-dev/rspress#1105 (comment)

And in my practice, 10 is too big when including remark-directive, but 5 is OK.

@wooorm
Copy link
Member

wooorm commented Jan 16, 2025

I think the solution is for rspress to update?

I am not sure what practice you mean with directives? I have not encountered problems with directives.
You propose changing 10 to 5? Why?

@JounQin
Copy link
Member Author

JounQin commented Jan 16, 2025

I think the solution is for rspress to update?

Indeed, but that requires a lot of work and is a breaking change for the rspress ecosystem, so that would not happen quickly, like prettier.

I am not sure what practice you mean with directives? I have not encountered problems with directives.

You propose changing 10 to 5? Why?

I'm just saying for v5, when I import remark-directive, do nothing, together with remark-mdx, tsc breaks, and using 10 doesn't help, 5 works.

I'm currently using local patch to workaround.

@wooorm
Copy link
Member

wooorm commented Jan 16, 2025

It’s been 16 months. There have been so many bug fixes in so many packages. I cannot backport everything. Software moves, I get that folks can’t always update immediately but rspress really needs to move fast than, what, every 2 or 3 years?

I sometimes backport fixes. But often because runtime code actually crashes. And I don’t think I have done that after more than a year.

I'm currently using local patch to workaround.

Why has no other rspress user mentioned this problem before?

Why can you not use the latest version of this package?

together with remark-mdx, tsc breaks

That sounds like a problem. Are you using an outdated tsc too?

@JounQin
Copy link
Member Author

JounQin commented Jan 16, 2025

I sometimes backport fixes. But often because runtime code actually crashes. And I don’t think I have done that after more than a year.

I totally understand, that's why I'm sending the PR instead of requesting you to fix it. But this issue makes tsc unusable which blocks VSCode. So have to fix it, the local fix works, and I hope to benefit more related users.

Why has no other rspress user mentioned this problem before?

Not every rspress user makes custom remark plugins, remark-directive is not used directly by rspress, I meet this issue when I start developing a plugin for our own use case.

Why can you not use the latest version of this package?

As I mentioned, rspress is using an old version of unified ecosystem, @types/mdast is v3, I'm not directly depending on this package, but unist-util-visit@4, upgrading unist-util-visit itself is not compatible with other remark plugins, or @types/mdast, right?

That sounds like a problem. Are you using an outdated tsc too?

Nope, latest v5.7.3

@wooorm
Copy link
Member

wooorm commented Jan 16, 2025

upgrading unist-util-visit itself is not compatible with other remark plugins, or @types/mdast, right?

Have you tried?

See the changelog: https://github.com/syntax-tree/unist-util-visit-parents/releases.

I think you can use latest versions.

@JounQin
Copy link
Member Author

JounQin commented Jan 16, 2025

Installing the new unist-util-visit does work but it also introduces duplicate different versions dependencies with rspress which I want to avoid, if fixing on old version is unacceptable I'll continue to use local patch instead.

@JounQin JounQin closed this Jan 16, 2025

This comment has been minimized.

@wooorm wooorm deleted the fix/types branch January 20, 2025 10:46
@wooorm
Copy link
Member

wooorm commented Jan 20, 2025

but it also introduces duplicate different versions

You can use a resolution if you want to work around that, but it’s a feature of the javascript world that 2 versions can exist.

I recommend to get rspress to update. There are many bug fixes that they miss.
I do not have the time to maintain different release lines for many years

@wooorm wooorm added the 👀 no/external This makes more sense somewhere else label Jan 20, 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 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

2 participants