peggyjs / peggy

Peggy: Parser generator for JavaScript
https://peggyjs.org/
MIT License
914 stars 65 forks source link

Add typescript types to bin #538

Closed hildjj closed 1 month ago

hildjj commented 1 month ago

The real news here is outputting .d.ts files next to the emitted .js file.

hildjj commented 1 month ago

Given the quantity of changes I've attached a patch file with every change I would make to the .d.ts file

I'm going to edit in all of your suggested changes myself so I can reinforce my learning, if that's ok with you.

LukeAbby commented 1 month ago

I've got no problem with editing by hand! I just didn't want to be overwhelming.

hildjj commented 1 month ago

@LukeAbby can you look at the current version and mark each of our sub-discussions as "resolved" if I've gotten them taken care of? I continue to be open to any other nits you find -- I appreciate your help!

LukeAbby commented 1 month ago

Hey! @hildjj sorry, I guess I wasn't seeing the ping because of how my GitHub notifications are set up. I actually only checked the issue again to see how things are moving. I think my only comment left is about the typing of GrammarSource;

You currently have

export type GrammarSource = string | GrammarSourceObject | unknown;

But like you said, I think we can probably be more specific! If you can give me a list of everything you think it can be then I could write out the type and I think that'd be the last of it.

hildjj commented 1 month ago

I think the Stringable change here captures what's important. Will this work for you?

LukeAbby commented 1 month ago

It probably still catches more than you expect; for example it allows Symbol,number, object, arrays, effectively everything except something like Object.create(null);.

Besides that it looks like it should! 🎉🎉🎉

Great job with the persistence with getting this right, I hope I was of help! If you ever want help again you can ping me and I'd be happy to take a look.

hildjj commented 1 month ago

OK, I'm going to write docs for this, then land it.

hildjj commented 1 month ago

@LukeAbby this should be almost done.

One last thing, can you please change dts: false to dts: true in src/opts.mjs, then run npm build? tsc is getting stuck in an infinite loop trying to deal with the generated .d.ts file. I don't think it's related to the returnTypes; switch those to any and the error still happens. It looks like it's actually trying to infer types from parser.js even though there is a .d.ts file, which is surprising to me.

LukeAbby commented 1 month ago

Hi. Sorry, I got busy so I didn't swing back around to this as fast as I should've. I spent a bit of time reducing down the problem space. You can reproduce with just this program:

var a$b = {};
var c, d;
d = a$b;
while (d !== a$b);
while ((c = a$b != a$b)) c.e;

(Obviously when reduced down to this simple it's absolutely absurd)

Looks like this was actually fixed in this commit: https://github.com/microsoft/TypeScript/commit/71fb8641386171f1fd869d769433260e7649c23e so presumably the next TypeScript version won't have this problem. You can also downgrade to 5.4 which doesn't have this problem either. I ended up reporting this in this issue: https://github.com/microsoft/TypeScript/issues/59594 but I only asked them to add a regression test since it was already fixed.

hildjj commented 1 month ago

Thanks, @LukeAbby. I think the least-bad option is to keep this PR the way it is, then flip false to true for dts when the next TS rev comes out.