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

Converts project to TypeScript #449

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ralexmatthews
Copy link
Contributor

@ralexmatthews ralexmatthews commented May 15, 2024

Description

This converts the project to TypeScript. In order to do this, a handful of kind of major-ish things needed to be done.

Add TypeScript build step

Instead of trying to get Webpack/Babel to compile the TypeScript, I made it so that TypeScript did the first compilation from TS -> JS. Doing it this way was both simpler, and we needed to do a TypeScript compilation anyway to create the TypeDefs. This process includes:

  • Hand-copy types/ TypeScript types to respective folders in services/ folder
  • Delete hardcoded types/ folder and add types to .gitignore
  • Add TS Build step to go from src/ to out-tmp and types-tmp
    • These -tmp dirs are needed because the source relies on package.json, and so TypeScript wants to build to out/src/... instead.
  • Once these are done, we copy out-tmp/src to out and types-tmp/src to types
  • Change webpack to build from the out dir instead of src dir

Tests

Because the tests are JS and relied on the JS src code, I just changed the tests to instead use the out dir, since it should be effectively the same. This does mean that in order to run the tests, you need to build first.

Models

Now that we are using TypeScript, I thought the models directory was kind of redundant. Since they were mostly just "structs", classes with no methods or anything, they functioned the same as just a regular object would. And now that we have the types, we get most of the value they were giving anyway. Also this reduces the bundle size non-trivially.

I realize this is a huge change, and if it can't be accepted all at once, that is fine, and I am more than willing to work with you to do this incrementally or whatever

Testing

I ran the tests with my EasyPost API keys and they all passed.

EASYPOST_TEST_API_KEY="EZTK..." EASYPOST_PROD_API_KEY="EZAK..." npm run test

Pull Request Type

Please select the option(s) that are relevant to this PR.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement (fixing a typo, updating readme, renaming a variable name, etc)

@ralexmatthews ralexmatthews requested a review from a team May 15, 2024 22:18
Copy link
Member

@Justintime50 Justintime50 left a comment

Choose a reason for hiding this comment

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

Sweet, thanks for putting this up! We'll want to determine if this is the route we want to take longterm before fully committing. It's also a massive PR, is there an approach we can discuss to transition the project piece by piece or batches at a time?

Blocking because there's some decisions that will need to be made here about next steps.

@ralexmatthews
Copy link
Contributor Author

ralexmatthews commented May 15, 2024

Hmmm... I am now realizing the way the linting works in this project doesn't allow for typescript support trivially. It looks like it is symlink-ing its eslint configs from the examples repo? If I am reading this right, that falls back to babel/webpack for parsing files, which is not set up here. May need to look deeper into that...

We'll want to determine if this is the route we want to take longterm before fully committing

Is this meaning like if we want to move to TypeScript? Or what route we want to take to move to to TypeScript?

It's also a massive PR, is there an approach we can discuss to transition the project piece by piece or batches at a time?

Hmmm... if you'd rather, I can see if there is a way to maybe allow for JS/TS together in the src dir, and just put up PR after PR converting the files to TypeScript? It'd be a shame to have to make like 15 different versions for implementing TS tho since the work would already be done... but I'm down for whatever.

@Justintime50
Copy link
Member

Is this meaning like if we want to move to TypeScript? Or what route we want to take to move to to TypeScript?

Correct, we didn't have concrete plans to move this to Typescript though it was discussed in the past. For instance, does our set of tooling work with Typescript, are maintaining devs prepared to support Typescript instead of JS, how does it affect the build process longterm, what downstream affects does this have for end-users and how can we pair that with other breaking changes in a single major release, etc. There's a lot to consider with migrating a project from one language to another (superset).

Hmmm... if you'd rather, I can see if there is a way to maybe allow for JS/TS together in the src dir, and just put up PR after PR converting the files to TypeScript? It'd be a shame to have to make like 15 different versions for implementing TS tho since the work would already be done... but I'm down for whatever.

Yeah I think we should discuss options internally as a team. As it stands, 4k and 6k line changes is just way too big to safely review in one go. When we are preparing a new major release of our libs, we typically create a new major version branch and merge in the various feature branches to it over the course of weeks/months while keeping the master branch as the current version. This way we can slowly rollup a bunch of changes while validating it separately from the master branch as we prep the next release. Something like this would fall into that category.

I'm not opposed to swapping to Typescript, I do feel there are a few unanswered questions we should explore and then determine how best to proceed so it's easy for you (can we avoid 15 PRs) but easy for reviewers too (can we batch things into 3 or 4 PRs).

@nwithan8 nwithan8 marked this pull request as draft May 21, 2024 22:20
@ralexmatthews
Copy link
Contributor Author

Hey, so after some talks not in this PR, it looks like it is generally accepted that we will be moving forward with TypeScript? If that is the case, how can I get this diff into an acceptable state to move forward?

@Justintime50
Copy link
Member

@ralexmatthews I'll follow up with you on Slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants