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

refactor: replace synckit with make-synchronized #1366

Merged
merged 1 commit into from
Mar 22, 2025

Conversation

fisker
Copy link
Contributor

@fisker fisker commented Mar 22, 2025

make-synchronized has a simpler API to use.

@fisker fisker marked this pull request as ready for review March 22, 2025 17:42
@fisker fisker force-pushed the make-synchronized branch from 92f05b7 to e44ca59 Compare March 22, 2025 17:45
@brettz9 brettz9 merged commit f8e9960 into gajus:main Mar 22, 2025
5 checks passed
@fisker fisker deleted the make-synchronized branch March 22, 2025 20:55
@JounQin
Copy link

JounQin commented Mar 23, 2025

Again, performance should be taken into account, synckit has better performance, TypeScript and native PnP support, etc.

This is not really cool.

@fisker
Copy link
Contributor Author

fisker commented Mar 23, 2025

I don't really care people using my package or not, just trying to help.

I don't even know what "TypeScript and native PnP support" mean, as I understand they all use Node.js loaders, and worker automatically inherit them, libs don't need do anything. Anyway, make-synchronized already tested loader support.

Feel feel to revert.

@JounQin
Copy link

JounQin commented Mar 23, 2025

I don't even know what "TypeScript and native PnP support" mean

See jest failng test case: jestjs/jest#15546

@fisker
Copy link
Contributor Author

fisker commented Mar 23, 2025

That's not related at all...

@fisker
Copy link
Contributor Author

fisker commented Mar 23, 2025

This is not the right place to discuss this. Feel free to open an issue at make-synchronized if you find bugs.

@JounQin
Copy link

JounQin commented Mar 23, 2025

That's not related at all...

Not sure what's your meaning:

/Users/runner/work/jest/jest/packages/jest-snapshot/src/__tests__/printSnapshot.test.ts

  ● Test suite failed to run

    TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /Users/runner/work/jest/jest/packages/jest-snapshot/src/prettier.ts

as I understand they all use Node.js loaders, and worker automatically inherit them, libs don't need do anything

This is incorrect for PnP at least. There is no auto inherit at all.

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this pull request Mar 23, 2025
@brettz9
Copy link
Collaborator

brettz9 commented Mar 23, 2025

Before I revert, do we have some benchmarks showing the better performance?

@JounQin
Copy link

JounQin commented Mar 23, 2025

@brettz9 See https://github.com/un-ts/synckit#benchmark

Sometimes even better: https://github.com/un-ts/synckit/actions/runs/14015583415/job/39240872165#step:7:1

brettz9 added a commit that referenced this pull request Mar 23, 2025
Copy link

🎉 This issue has been resolved in version 50.6.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

@fisker
Copy link
Contributor Author

fisker commented Mar 24, 2025

@brettz9

I found something interesting when playing with parse-imports.

parseImportsSync from parse-imports is much faster.

┌─────────┬────────────────────────────────┬──────────────────┬───────────────────┬────────────────────────┬────────────────────────┬─────────┐
│ (index) │ Task name                      │ Latency avg (ns) │ Latency med (ns)  │ Throughput avg (ops/s) │ Throughput med (ops/s) │ Samples │
├─────────┼────────────────────────────────┼──────────────────┼───────────────────┼────────────────────────┼────────────────────────┼─────────┤
│ 0       │ '[SYNC ] parseImportsSync'     │ '42196 ± 0.80%'  │ '39284 ± 371.00'  │ '24703 ± 0.14%'        │ '25456 ± 243'          │ 23699   │
│ 1       │ '[ASYNC] parseImports'         │ '41969 ± 0.66%'  │ '39824 ± 340.00'  │ '24599 ± 0.11%'        │ '25110 ± 213'          │ 23828   │
│ 2       │ '[SYNC ] synckit'              │ '144921 ± 1.14%' │ '131486 ± 7604.0' │ '7172 ± 0.30%'         │ '7605 ± 460'           │ 6901    │
│ 3       │ '[SYNC ] makeSynchronized'     │ '137073 ± 0.82%' │ '126431 ± 9502.0' │ '7532 ± 0.29%'         │ '7909 ± 633'           │ 7296    │
│ 4       │ '[SYNC ] makeSynchronized 0.3' │ '186558 ± 1.29%' │ '185787 ± 21090'  │ '5587 ± 0.38%'         │ '5383 ± 590'           │ 5361    │
└─────────┴────────────────────────────────┴──────────────────┴───────────────────┴────────────────────────┴────────────────────────┴─────────┘

I understand it should wait for wasmLoadPromise, I think you can do this, if you care about perf

let parse = functionSynchronizedWithWhatever

wasmLoadPromise.then(() => {
  // Swith parse to the original one
  pares = parseImportsPackage.parseImportsSync
})

const parseImports = code => parse(code)

@JounQin
Copy link

JounQin commented Mar 24, 2025

parse-imports seems a nice package, we can also use it in eslint-plugin-import-x. cc @SukkaW

@SukkaW
Copy link

SukkaW commented Mar 24, 2025

parse-imports

No, I'd rather use the battle-tested es-module-lexer directly over this wrapper package parse-imports. In fact, I have already made the es-module-lexer sync in guybedford/es-module-lexer#185.

I created the proposal months ago, but haven't got time to get my hands on implementing it. un-ts/eslint-plugin-import-x#201

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants