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

feat: Add Paseri to benchmarks #1503

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vbudovski
Copy link

This adds the Paseri parsing library to the benchmark. In order to support this, and any future ESM-only libraries, this benchmarks package was updated to use ESM.

save-exact=true
@jsr:registry=https://npm.jsr.io
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just adds a link to the jsr.io package registry, which is a modern version of the npm registry. Paseri is hosted there only, not on npm. https://jsr.io/#why-jsr

"outDir": "build"
"module": "ESNext",
"outDir": "build",
"allowImportingTsExtensions": false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain what these changes do

Copy link
Author

@vbudovski vbudovski Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we inherited the tsconfig we need to flip the allowImportingTsExtensions setting, because it won’t work when transpiling, only for generating types. https://www.typescriptlang.org/tsconfig/#allowImportingTsExtensions

The other two changes here are to make this module ESM to match the top-level module, since we can’t mix commonjs/ESM.

@DarkGL
Copy link
Collaborator

DarkGL commented Dec 23, 2024

As those are pretty big changes, could you please explain what those building option do ?

@vbudovski
Copy link
Author

As those are pretty big changes, could you please explain what those building option do ?

There are generally 2 types of changes that were made:

  1. If a parser library has an ESM version pre-built, we reference that instead of the default commonjs entry point.
  2. If a library needs a build step, we convert the existing commonjs output configuration into one that produces ESM code instead.

Since we can’t mix import types, we needed to convert everything to ESM.

@DarkGL
Copy link
Collaborator

DarkGL commented Dec 23, 2024

Tests have failed. Could you check it out?

@moltar
Copy link
Owner

moltar commented Dec 23, 2024

@vbudovski We appreciate your contribution, and it is obvious that a lot of work went into this PR to make it work.

As much as I'd love to merge this, I feel like many of the changes are unrelated, even though they are desirable (due to the ESM requirements), which I think is currently outside the scope of adding a library.

There's a long outstanding issue (#899) to convert this repo to a monorepo, which could support package-specific settings, yet retain the structure.

I will try to address this over the holidays, and perhaps, convert the entire repo in one swoop.

Let's wait until we merge this.

@vbudovski
Copy link
Author

@vbudovski We appreciate your contribution, and it is obvious that a lot of work went into this PR to make it work.

As much as I'd love to merge this, I feel like many of the changes are unrelated, even though they are desirable (due to the ESM requirements), which I think is currently outside the scope of adding a library.

There's a long outstanding issue (#899) to convert this repo to a monorepo, which could support package-specific settings, yet retain the structure.

I will try to address this over the holidays, and perhaps, convert the entire repo in one swoop.

Let's wait until we merge this.

Yeah. I didn’t expect that I’d need to make so many changes. I’d be happy to split out the ESM conversion into a separate PR if that would make it more palatable? Otherwise, happy for you to do the monorepo conversion.

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

Successfully merging this pull request may close these issues.

3 participants