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

Replaces Webpack/Babel/Mocha with Vite/Vitest #485

Merged
merged 10 commits into from
Jan 6, 2025
Merged

Replaces Webpack/Babel/Mocha with Vite/Vitest #485

merged 10 commits into from
Jan 6, 2025

Conversation

ralexmatthews
Copy link
Contributor

@ralexmatthews ralexmatthews commented Jan 2, 2025

Description

Sorry, I was a dummy and accidentally closed #484 by deleting my own fork 🙃 so this is the exact same code, just on an easypost branch.

I decided to start with a much smaller problem to tackle, so this only removes Webpack/Babel/Mocha with Vite/Vitest. I chose Vite/Vitest because that it what Easy-UI uses for its builds and tests.

There is a lot of files changed, but all changes are actually pretty minimal. All this does really is:

  • Remove all webpack/babel/mocha dependencies and related files
  • Installs Vite/Vitest and creates their config files
  • Convert the tests to support Vitest
    • changes the way Polly is set up
    • remove all this. references in the tests
    • all cassettes work in place and none needed to change
  • Makes some changes to github workflows
    • Adds a "test-node-compatibility" test that checks importing and using of the library
    • We don't want to just run the test suite because vite/vitest requires node v18+, but can generate output for arbitrarily low node versions

And thats effectively it. All commands still work as is. There are no changes to the source, and the output file is effectively the same. This does mean that it hasn't fixed any of the weird issues with using the project in Next.js and stuff. That can come at a later date since that will likely require breaking changes to fix.

Overall, this removes like half of the project's deps, and with it, the security vulnerabilities in exchange for a modern toolset.

Testing

All unit tests are passing. In added another test/check specifically for checking compatibility with older node versions. It basically just imports the project and tries to create a test shipment. This makes it so we can test compatibility with old node versions while not requiring the toolchain to support old node versions.

const EasyPostClient = require('../..');

const test = async () => {
  const client = new EasyPostClient('apiKey');

  if (!client.baseUrl.match(/^(http|https):\/\/api.easypost.com/gm)) {
    process.exit(1);
  }
};

test();

I also tested it with using actual API calls with a minimal project using node 12 and 22 and saw it working. I also checked node 10 and confirmed that did not work.

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 as a code owner January 2, 2025 16:34
nwithan8
nwithan8 previously approved these changes Jan 2, 2025
@Justintime50
Copy link
Member

@ralexmatthews if you can patch up the code scanning issue, I'll take a look

@ralexmatthews
Copy link
Contributor Author

@Justintime50 fixed 👍

test/node_compatibility/index.js Fixed Show resolved Hide resolved
@ralexmatthews
Copy link
Contributor Author

Dang it lol

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.

Last time we attempted to overhaul the build system, we ran into issues once the final product was built, it couldn't be imported/referenced properly. Have we ensured that that issue is not a problem after these changes by running and E2E test by trying to import and use the final output of Vite?

.github/workflows/ci.yml Show resolved Hide resolved
package.json Show resolved Hide resolved
test/helpers/setup_polly.js Outdated Show resolved Hide resolved
vite.config.js Show resolved Hide resolved
@ralexmatthews
Copy link
Contributor Author

Have we ensured that that issue is not a problem after these changes by running and E2E test by trying to import and use the final output of Vite?

Yea, I tested it with using actual API calls with a minimal project using node 12 and 22 and saw it working. I also checked node 10 and confirmed that did not work.

Also, thats what the node-compatibility CI check is supposed to check for.

So as far as I can tell, it has full compatibility with the current setup

@nwithan8 nwithan8 self-requested a review January 3, 2025 19:12
@nwithan8 nwithan8 merged commit 5e4b432 into master Jan 6, 2025
17 checks passed
@nwithan8 nwithan8 deleted the use-vite branch January 6, 2025 18:09
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.

3 participants