Skip to content
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

[PR] Overall Typescript refactor #277

Merged
merged 107 commits into from
May 14, 2024
Merged

[PR] Overall Typescript refactor #277

merged 107 commits into from
May 14, 2024

Conversation

LuchoTurtle
Copy link
Member

@LuchoTurtle LuchoTurtle commented Oct 25, 2022

Seeing that @thomaux already did an awesome job with implementing type definitions, I wanted to learn more about generics in Typescript so I'm attempting to rewrite the main index.js file in Typescript so it's friendlier for everyone 😄 .

This is very much a work in progress, as I'm trying to use the pre-existing type definitions in the current package. There's much to be done but I guess this is a start. I'm running every modification against tests so functions that are typed so far should remain functional.

Some tasks:

  • Export type definitions (already implemented by @thomaux )
  • Rewrite functions to make use of type definitions (index.js -> index.ts)
  • Rewrite tests to make use of ts.

If anyone would like to chime in and give some feedback, that would be lovely!

In-progress of typing out the function parameters.
After getting through this method, the rest should be fairly simple.
Typing mock future. This can pass over to the rest of the functions.
@LuchoTurtle LuchoTurtle added enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person epic A feature idea that is large enough to require a sprint (5 days) or more and has smaller sub-issues. labels Oct 25, 2022
@nelsonic
Copy link
Member

@LuchoTurtle thanks very much for opening this PR as we discussed. 👌
100% on-board with a TypeScript re-write of this module for greater TS support. 🎉
(obvs it will still be compiled to JS for full backward compatibility ...)
Please consider opening an issue to outline the work that needs to be done. 🆕
Preferably with a checklist so that it can be worked on over multiple sessions. 👩‍💻
Thanks! 🙏

@thomaux
Copy link
Collaborator

thomaux commented Oct 25, 2022

Great work, happy to help if time permits and where needed. Seeing as there are some hiccups with the current typings (specifically concerning nested services etc.) don't be too worried about reusing those.

The tests should indeed be a good safety net to avoid regression.

@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Oct 25, 2022

@thomaux thank you, sir! I was guided by your awesome type definitions. I try to reuse as much as possible but I gotta admit I'm stumped on some aspects, especially when I constantly get M cannot be used as an index type.

Again, I'm new to this level of TS generics, so I'm just trying to learn. But thank you for the feedback, I'll take that into account 😄

@LuchoTurtle LuchoTurtle marked this pull request as draft October 31, 2022 10:57
@nelsonic
Copy link
Member

@nelsonic
Copy link
Member

@LuchoTurtle remind me why this was paused? ⏸️ 💭 ⏳

@LuchoTurtle
Copy link
Member Author

@nelsonic I was only meant to work on this whenever I "had the time" or "didn't have any outstanding issues to work at", which really hasn't happened since.
Additionally, I'm not particularly well-versed in TS generic types and I keep stumbling upon blocking errors that are a bit beyond me. I'm thinking of doing something like https://www.totaltypescript.com/ to actually sort this PR out but it's a big task and there are other more important tasks to tackle first 👌

Added the changes on the index.js to coincide with the ones on TS. All JS tests should STILL work, to avoid regressions.
@nelsonic
Copy link
Member

@LuchoTurtle Thanks for pairing on this during the morning today. 🧑‍💻 👌
Even if it felt like "slow progress", don't worry. 💭
It's a worthy effort if you want to add TypeScript skills 💪
& maintaining/refactoring a highly popular library to your list. ✅

@LuchoTurtle
Copy link
Member Author

@nelsonic

Should it be a major version e.g. v6? 💭

Without a doubt. Because of the combination of the dependencies being updated and how the code is being transpiled from TS to JS, support only works for Node v18 upwards.

@nelsonic
Copy link
Member

Ok, cool. Do you want to publish the new version? 📦

@LuchoTurtle
Copy link
Member Author

I feel like you should be the one to do it, I don't want to mess anything up since it's a package that is highly used :p

@nelsonic
Copy link
Member

# Conflicts:
#	package-lock.json
#	package.json
@LuchoTurtle
Copy link
Member Author

I'm not sure how I'd go about it. Wouldn't this need to be merged first for me to create a new version of the package in npm?

@nelsonic
Copy link
Member

Nah, you can create & publish a new version on a branch. 📦 🚀

Simply run:

npm version major -m 'Typescript Refactor. see: https://github.com/dwyl/aws-sdk-mock/pull/277"
npm publish
git push && git push --tags

Invited you to maintain:
image

Will merge as soon as you publish. :shipit:
If anything goes "wrong" (highly doubt it) message me on Signal. 💬

@nelsonic nelsonic assigned LuchoTurtle and unassigned nelsonic May 2, 2024
@LuchoTurtle
Copy link
Member Author

I'm intent on publishing this later. I'm at the airport right now and I've fast-forwarded this branch to the main. I'm publishing it now because I'm hesitant about having some issues. If that's the case, I could only get back to this in 24 hours.

@nelsonic
Copy link
Member

Do it. I'm here and check GitHub regularly for messages. Not many people will "upgrade" that quickly.

@LuchoTurtle
Copy link
Member Author

Okay, doing it now (after updating package-lock.json.

LuchoTurtle added a commit that referenced this pull request May 13, 2024
@LuchoTurtle
Copy link
Member Author

@nelsonic https://www.npmjs.com/package/aws-sdk-mock version 6.0.0 published.

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

Great work @LuchoTurtle 🎉
Thanks for publishing the new version to NPM. 📦 🚀
Merging. :shipit:

@nelsonic nelsonic merged commit d9fc691 into main May 14, 2024
10 checks passed
@nelsonic nelsonic deleted the typescript-refactor branch May 14, 2024 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed enhancement New feature or enhancement of existing functionality epic A feature idea that is large enough to require a sprint (5 days) or more and has smaller sub-issues. priority-1 Highest priority issue. This is costing us money every minute that passes. technical A technical issue that requires understanding of the code, infrastructure or dependencies
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants