Skip to content

Improve OXC Support #76

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 9 commits into
base: main
Choose a base branch
from
Open

Conversation

andylovescode
Copy link
Contributor

This pull request makes the following improvements:

  1. Previously, when we were printing ParenthesizedExpressions, we would just naively print the node inside of it, instead of parenthesizing it. I am not quite sure why, but that is now fixed.[1]
  2. I added a helper called resolveIndexedLocations
    Why is this needed? Sourcemaps don't let you explicitly specify the index of a location, but rather require a row and column number. This would be fine (if not slightly silly), if it weren't for the output of `oxc-parser` containing `start` and `end` fields, which use indicies rather than row and column numbers. Thus, we must have the original source code to turn said indicies into row and column numbers, which is not guranteed with the typical `print` api, so I created a wrapper around normal languages (which requires the source code to create), that adds a catch-all function to support the `oxc` style nodes. If I had more time, I could've made this more concise, sorry.

And of course, I added changesets for these as well.

[1]I didn't just do this because it seemed off, it was actively causing problems because of how oxc-parser parses IIFEs

As `oxc-parser` will emit `ParenthesizedExpression`s, we need to wrap
them in parenthesis, otherwise the printed code may be unparsable, for
example in the case of the following:

```typescript
// Parse using `oxc-parser
(()=>{
    console.log("I am an IIFE");
})();
```

Which was previously printed as

```
   ()=>{
       console.log("I am an IIFE");
   }();
// ^ lack of paren makes this syntactically invalid
```
`oxc-parser` emits AST nodes that do not contain `loc` fields, but
rather `start` and `end` indicies.

Unfortunately, that means we cannot add support for those by default, as
due to the sourcemap specification being from the depths of hell, we
must provide a line and column number, and thus must have the original
source code to convert from the index to line-and-column.
Copy link

changeset-bot bot commented Jul 5, 2025

🦋 Changeset detected

Latest commit: 46d8108

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
esrap Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

pkg-pr-new bot commented Jul 5, 2025

npm i https://pkg.pr.new/sveltejs/esrap@76

commit: 46d8108

@andylovescode
Copy link
Contributor Author

It appears that there is a sourceMapContent prop I can use to get indices instead of adding a weird interceptor. Will update code. Making this a draft for now.

@andylovescode
Copy link
Contributor Author

Looks like (to my knowledge), I can't make this a draft myself. Hopefully by the time you see this it will be done anyways.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

Converted it to a draft for you!

@andylovescode andylovescode marked this pull request as ready for review July 8, 2025 02:53
@andylovescode
Copy link
Contributor Author

I asked for it to be made a draft before I made the changes I had liked to make, now I believe it ready to merge (with some review).

@andylovescode
Copy link
Contributor Author

This is ready for review (and has been for a while), sorry for the mixup with me asking for it to be made a draft before I had finished it.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

There's a lot of seemingly unrelated formatting changes. Did you run prettier before committing? (we should add a lint check to the CI)

src/context.js Outdated
@@ -104,13 +122,22 @@ export class Context {
throw new Error(message);
}

if (node.start && this.#sourceText) {
Copy link
Member

Choose a reason for hiding this comment

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

Could fail if start is 0 (same below for node.end though that's even less likely)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed

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