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

Eliminate require() in favor of import #79

Closed
wants to merge 1 commit into from

Conversation

deldrid1
Copy link

This PR fixes some legacy require()'s and runtime imports littered in the code base that were causing issues with Webpack (TypeError: axios is not a function at runtime) in a large ESM node 20 project that still includes some CommonJS package dependencies. I suspect it may help address #53

I don't expect it to be merged as is (I haven't updated the version or anything), but wanted to put it here in case the team wants to pull it in!

In addition to replacing require with import, this PR also:

  • Bumps axios version to latest
  • adds a prepare script to package.json so that my GitHub install via for my project is unblocked and node_modules/klaviyo-node-api folder has a dist folder
    • This can probably be rolled back and isn't required to merge

- Bump axios version to latest
- add prepare script
@@ -1,3 +1,6 @@
import querystring from 'querystring';
Copy link
Author

Choose a reason for hiding this comment

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

tree-shaking should take care of ignoring unused modules. These are native anyway, so I don't see any reason to do a complicated runtime import.

Copy link
Author

Choose a reason for hiding this comment

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

There's also no benefit of prefixing with node: to my knowledge - I left it out for compatibility as I didn't feel like studying config files to understand what is appropriate here :)

@sanfordj
Copy link
Contributor

Thank you for your contribution! Due to the way we develop our SDKs, we'll need to make this change in a different internal repo. We will update this thread when it is pulled in and released!

@sanfordj
Copy link
Contributor

meant to mention, for now im going to close the pull request for internal tracking purposes

@sanfordj sanfordj closed this Aug 29, 2024
@deldrid1
Copy link
Author

That's what I figured. Thanks!

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