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

Removes Webpack/Babel in favor of SWC #461

Merged
merged 19 commits into from
Jul 24, 2024
Merged

Conversation

ralexmatthews
Copy link
Contributor

Description

In its current state, the node library uses a webpack + babel build pipeline to bundle the whole library into one big CommonJS bundle. It uses maybe a dozen different webpack and babel settings, configs, plugins, etc to do this as well. This is probably not ideal for several reasons.

  • Since this should only be used in Node.js, not in the browser, this does not need bundled, so Webpack isn't needed altogether
  • Babel, while popular, isn't super fast, and has lots of plugins needed to get the settings we want

Enter SWC, the Babel replacement written in Rust. It is a dependency of Turbopack, Vercel's new transpiler/bundler for Next.js, and seems to be the current future.

As part of this move to SWC, I have also created both a CommonJS and ES6 build of the app to support the movement of the community away from CommonJS.

Getting rid of all the babel deps also removes these warnings when running npm install:

npm WARN deprecated @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-export-namespace-from instead.
npm WARN deprecated @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-class-properties instead.
npm WARN deprecated @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-unicode-property-regex instead.
npm WARN deprecated [email protected]: core-js@<3.23.3 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Some versions have web compatibility issues. Please, upgrade your dependencies to the actual version of core-js.

Overall, this greatly reduces the number of dependencies, config files, and improves support for CJS and ESM projects.

Also fixes #446. I was able to replicate the error by creating a new Nest app, and by changing the version to this, it fixes that error.

Also fixes #456 by exporting all errors and models from the root.

Testing

All tests are still passing, thanks to swapping out babel/register to swc-node/register

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)

Comment on lines +25 to +29
- name: Use Node.js ${{ matrix.node-version }} to test importing and using
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
- run: make test-import-and-use
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we actually care that you can build, test, and generally dev on the project in Node.js 12. This builds the app on a modern version, then uses the older version to run a small script that imports and attempts to use the lib.

nwithan8
nwithan8 previously approved these changes Jul 23, 2024
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@ralexmatthews ralexmatthews marked this pull request as ready for review July 23, 2024 18:39
@ralexmatthews ralexmatthews requested a review from a team July 23, 2024 18:39
nwithan8
nwithan8 previously approved these changes Jul 23, 2024
nwithan8
nwithan8 previously approved these changes Jul 23, 2024
Justintime50
Justintime50 previously approved these changes Jul 24, 2024
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.

praise: I'm ecstatic to see this! 😃 Solid work, babel and webpack have been the bane of this lib for years...

Curious, does this break anything? I looked into removing babel a couple years back and it got very angry at me, it's possible that over the last couple years and the various refactors we've done we've slowly ripped out anything that previously was a problem but did want to check if there were any implications. We did previously remove the ability for this to run in the browser which may have been the sole thing needed to unblock this.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Comment on lines +20 to +25
"exports": {
".": {
"require": "./dist/cjs/index.js",
"import": "./dist/esm/index.js"
}
},
Copy link
Member

Choose a reason for hiding this comment

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

question: I assume this replaces the index.js file in the root? This accomplishes the same results though allowing users to require the entire library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, exactly. Starting in Node v12, you can use the conditional exports to specify what they should get when they use your package. Basically for both the CJS and ESM builds, the entire src directory is built and built in these folders, so if they are using an import EasyPostClient from "@easypost/api" it will import the "import"/esm build, and for older projects that do a const EasyPostClient = require("@easypost/api") it will use the "require"/cjs built assets.

@nwithan8 nwithan8 merged commit 64656fe into EasyPost:master Jul 24, 2024
14 checks passed
ralexmatthews added a commit that referenced this pull request Jul 24, 2024
nwithan8 pushed a commit that referenced this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants