node-pureimage icon indicating copy to clipboard operation
node-pureimage copied to clipboard

Typescript migration

Open josh-hemphill opened this issue 3 years ago • 9 comments

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [x] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • [ ] My code follows the code style of this project.
    • [x] Currently has some proof-of-concept style enforcement
  • [x] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [ ] I have added tests to cover my changes.
    • [x] Most tests migrated and working, but some are unclear if they're old or unused.
  • [ ] All new and existing tests passed.

Description

closes #125 - Typescript migration discussion and notes related #114 - Missing typescript types supersedes #81 - Migrate to typescript

Initial migration proof-of-concept. Most things work already, and it has some lingering files for automating things I had copied from my repo I had used as a template. I'll get those working if there's interest.

The tests that are failing seem to be very difficult to diagnose, I'll probably need help with those. There's incorrect pixels...

josh-hemphill avatar Nov 17 '21 09:11 josh-hemphill

This PR changes a whole lot of things, far too much for review. Could you make a PR with just the typescript changes, not all of the editor options and formatting changes (like adding semicolons to every line).

joshmarinacci avatar Nov 17 '21 18:11 joshmarinacci

Yeah, there's a lot of stuff I grabbed from my template repo that I hadn't cleaned up yet. I had added linting because as I was rewriting things it wasn't clear if I should or shouldn't be using semicolons because it was all mixed depending on when it was written, so I just set it so it would all auto-format on save so I didn't have to worry about it.

I reverted trying to support Deno. It would require having to bundle all dependencies and then require Deno users to still add Node compatibility layer themselves because opentype.js and I think some of the other dependencies rely on Node.js built-in/standard modules.

josh-hemphill avatar Nov 17 '21 20:11 josh-hemphill

I've removed most of the editor/formatting stuff. (only kept the .editorconfig since it's opt-in, has cross-editor support, and just informs line endings and indentation)

josh-hemphill avatar Nov 21 '21 20:11 josh-hemphill

Did you intend to delete context.js and text.js

joshmarinacci avatar Nov 24 '21 21:11 joshmarinacci

Yes. It's just replaced with a context.ts and text.ts, it didn't detect those as file name changes for some reason. 🤷

josh-hemphill avatar Nov 24 '21 22:11 josh-hemphill

It looks like there are some conflicts to resolve before it can be merged.

joshmarinacci avatar Nov 28 '21 18:11 joshmarinacci

Could you resolve the conflicts? Then we can try the merge again.

joshmarinacci avatar Jan 08 '22 22:01 joshmarinacci

There's still 3 failing tests that I have no clue on. And I probably still need to write a couple more test to help with covering any type-checks that had to be added.

josh-hemphill avatar Jan 10 '22 11:01 josh-hemphill

The individual commits now show the files being renamed and then edited, but merging only compares the final state to target branch, in which it considers the renamed files to be too considerably different and marks them as deletions and additions in the merge diff. If you want them to appear as renames, you'd have to use git rebase or merge follow options via the command line.

josh-hemphill avatar Jan 11 '22 01:01 josh-hemphill

I've created a new port to typescript using ESBuild and Vitest, but I copied most of the actual typescript translation from your branch. Thank you so much. Please try out the new 0.4.* version with all of your hard work in them.

joshmarinacci avatar Aug 13 '23 23:08 joshmarinacci