mike-lischke / antlr4ng

Next Generation TypeScript runtime for ANTLR4
Other
66 stars 11 forks source link

Convert utils to typescript #4

Closed OskarDamkjaer closed 9 months ago

OskarDamkjaer commented 9 months ago

I took another stab at converting at typescript convertion of a few simple files, like the example you linked on my other PR earlier. Let me know if these are the kind of changes that would be useful!

Feel free to wait to review if it's still to early for these kinds of changes, no rush

mike-lischke commented 9 months ago

@OskarDamkjaer Have you run the unit tests locally before creating the PR? The tests in the GH action take much longer than before. I wonder if that is only GH or some regression from the conversion?

OskarDamkjaer commented 9 months ago

@mike-lischke yes and I re-ran them just now to double check. No change in speed between my branch and master when I try. My guess would be to double check the node version used in ci. After I ran npm install the engine field got removed from the lock file, as it was removed from the main package json earlier but without running npm install

I've also seen the tests vary quite a bit in time taken from run to run locally

mike-lischke commented 9 months ago

Well, I removed the engine field, as that is only used for VS Code extension, or so I thought. But I ran the test locally and they were fine by then (and yes they vary a bit, depending on the load of your box - we are talking here about milliseconds), though. Maybe it's really something with GH that slows testing down so much. It's certainly a shared resource (docker, vm or whatever) so its speed also depends on other load.

OskarDamkjaer commented 9 months ago

I'm not sure if the field is used in ci or not, it's possible it doesn't matter 🤷 It could be worth looking into vitest and/or bun to see if we get faster in CI.

mike-lischke commented 9 months ago

I just ran the tests locally and they are way slower than before. Might have to do with your type guard in standardEqualsFunction (which also contains some formatting problems and a type, how did I miss that 😳?

The engines field made no difference btw.

OskarDamkjaer commented 9 months ago

I saw a +/- second in variability between test runs on the same branch when I tested locally, so I didn't see any change between master and this branch. I can look into it closer and re-write the check to be less type safe and faster if you like.

I'd be good to get some automatic formatting set up with eslint, and have it run on commit with something like lint-staged to avoid having to look for formatting issues on PRs.

mike-lischke commented 9 months ago

No worries. I'm on the change already, also collecting all the small functions into a single file. Formatting happens automatically if you enable VS Code to let it do that on save (that's what I have enabled).

OskarDamkjaer commented 9 months ago

Alright, thanks! I had different formatting on save (prettier) so I disabled it altogether, I'll re-enable the default vscode one for this project.

Btw it's possible to commit a .vscode folder to set default settings for people who open the project (format on save with built in formatter and auto fix eslint problems).

mike-lischke commented 9 months ago

OK, back to normal.

btw. in case you wonder: I left in the ts-jest warnings, because they will automatically disappear, once we no longer have js files in the project.