search

peggyjs / peggy

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

Make generate-js.js ts clean #430

Closed markw65 closed 9 months ago

markw65 commented 1 year ago

While working on #429 I found that having @ts-check enabled on the new files was very helpful, so I decided to see how hard it would be to turn on for generate-js.js. This is the result.

I found several typing errors (either errors in jsdoc comments, or in peg.d.ts) - but these were just incorrect declarations - the code itself was fine (also, most of them were introduced by me, when I added source map support to generate-js.js).

I also found one possible bug (again, it was me that did it): A SourceNode was constructed with a child array consisting of strings and an array of SourceNodes (ie Array<string|Array<SourceNode>>). The type info says it should be a Array<string|SourceNode>. Apparently the code did work though, so who knows.

In any case, if this seems worthwhile, I'll be happy to put up similar pull requests for other files as I get time.

hildjj commented 1 year ago

I think this is the next one I want to land. I'll look at it later today, but could you do a quick rebase please?

markw65 commented 1 year ago

Rebased

reverofevil commented 1 year ago

JFYI there's a full rewrite in TS here

Feel free to steal anything from there, as I don't have time to update it to most recent version

markw65 commented 1 year ago

@reverofevil Nice.

From my point of view a full typescript rewrite would be much better. But I'd assumed that sticking with code that "just works" without a build step was probably a project requirement. If thats not the case, then sure...

reverofevil commented 1 year ago

@markw65 Whether it's a full rewrite or not, we still have to figure out how to make types work, and it's the same in either case.

As far as I understand, you plan to continue cleaning up peggy's codebase. In that case feel free to take anything from a (differently) cleaned up version of peggy.

markw65 commented 1 year ago

@hildjj Is there anything else you want here? I have the changes to stack.js if you'd like me to roll those into this pull request; or I can do that separately.

markw65 commented 11 months ago

@hildjj ping

hildjj commented 11 months ago

Sorry, have been traveling. Will get to this when I can, it's still first on the list.

hildjj commented 9 months ago

And of course I didn't get to this in a timely fashion. Apologies. Looking at it now.

hildjj commented 9 months ago

All this needs is a CHANGELOG.md entry, and it's ready to go.