Skip to content

perf(twoslash): calculate playgroundURL on-demand #3013

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

Open
wants to merge 4 commits into
base: v2
Choose a base branch
from

Conversation

antfu
Copy link
Contributor

@antfu antfu commented Jan 10, 2024

This would improve performance in usages like https://shikiji.netlify.app/packages/twoslash where the playgroundURL is never used, allowing saving bits to optionally not bundle lz-string

@jakebailey
Copy link
Member

It's been a couple months (sorry); I know that https://www.npmjs.com/package/twoslash claims to succeed the package here. Would it make sense to use that package now instead?

@antfu
Copy link
Contributor Author

antfu commented Mar 30, 2024

It wasn't blocking anything so no worries - I am more curious about what's the plan of the maintenance work for packages in this repo? For example, that data module is still on ts 4:

"typescript": "^4.4.4"
and haven't been published for a while now.

I would volunteer myself to do the minimal maintenance work (merging non-controversial PRs, widen version ranges, and fixing releases) for this project if needed (and if that's ok with Microsoft's policy) - Otherwise, let me know if if there is any other way I could help on this. I don't want to end up forking everything if there are better options to collaborate.

Thank you!

@jakebailey
Copy link
Member

Sorry for the long delay; right now we're pretty bogged down. I don't think we can give external users permission to do things on our repos, especially an official production website like this one, but accepting PRs is fine.

The main reason I hadn't personally merged this/other PRs that modify the non-website packages is just that we don't have an easy way to auto-bump package versions. I've had to merge PRs in batches, then manually determine which packages need version bumps (transitively) and then push that out. In other repos, we use changesets, which makes things a bit easier, though I haven't had time to integrate that workflow into this repo. Theoretically it should "just work" if we use the changesets action but have it skip publishing, such that merging the version bump PR causes the other stuff to be published.

@jakebailey
Copy link
Member

That being said, having to fix some stuff here recently, I think that we know concretely how to get things running again, so I think I could get some stuff fixed soon.

@jakebailey jakebailey closed this Jun 2, 2024
@jakebailey jakebailey reopened this Jun 2, 2024
@jakebailey jakebailey added the deploy-preview Enables automatic deployments to preview environments on a PR label Jun 4, 2024
@jakebailey
Copy link
Member

FYI the repo is now publishable again; sorry it took so long. I've taken the liberty to update your PRs and add changesets.

That being said, I think it may turn out to be a better move to deprecate @typescript/twoslash and start using twoslasher along with other updated deps.

@jakebailey
Copy link
Member

Though, this PR does appear to make the site not build; quite possible this approach is incompatible with gatsby somehow due to this prop not being serializable.

@orta
Copy link
Contributor

orta commented Jun 4, 2024

Yeah, IMO deprecating is the right call here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy-preview Enables automatic deployments to preview environments on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants