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

fix: Use url-join instead of urljoin #342

Merged
merged 2 commits into from
Jan 18, 2023

Conversation

adamsmasher
Copy link
Contributor

Which problem is this PR solving?

libhoney is pulling in a helper library called urljoin to fuse URL components together. urljoin expects to be run in a Node environment, and uses the Node standard library (notably the path module). Because of this dependency, libhoney fails for us when run under Webpack 5, which no longer bundles Node libraries in.

Short description of the changes

This PR swaps urljoin out for an alternative library with equivalent functionality, called url-join. url-join has no dependencies, and is newer than urljoin and still maintained.

Because it's new, it uses ES Modules; I needed to configure Jest to use Babel to transform it.

libhoney is pulling in a helper library called urljoin to fuse
URL components together. urljoin expects to be run in a Node
environment, and uses the Node standard library (notably the path module).
Because of this dependency, libhoney fails for us when run under Webpack 5,
which no longer bundles Node libraries in.

This PR swaps urljoin out for an alternative library with equivalent
functionality, called url-join. url-join has no dependencies, and is
newer than urljoin and still maintained.

Because it's new, it uses ES Modules; I needed to configure Jest to
use Babel to transform it.
@adamsmasher adamsmasher requested a review from a team January 10, 2023 18:28
@adamsmasher
Copy link
Contributor Author

Just saw that this is failing a check because it needs a proper prefix in its title. I'm going to update this to be a "fix:", although I'm not sure if it should be a "maint:" or "chore:" instead. Happy to make any changes requested 🙂

@adamsmasher adamsmasher changed the title Use url-join instead of urljoin fix: Use url-join instead of urljoin Jan 11, 2023
@vreynolds vreynolds added the status: oncall Flagged for awareness from Honeycomb Telemetry Oncall label Jan 11, 2023
Copy link
Contributor

@vreynolds vreynolds left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! The dependency change looks good. Can we leave the babel config format as is for the moment?

babel.config.js Outdated
@@ -1,4 +1,4 @@
{
module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, not sure if we want the config as JS or JSON. is this change necessary as part of this PR?

Copy link
Contributor Author

@adamsmasher adamsmasher Jan 18, 2023

Choose a reason for hiding this comment

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

So AFAICT Jest does not pick up the babel configuration in .babelrc, only in babel.config.js. Without this change, Jest won't have Babel process url-join, which causes tests to syntax error on the ES Modules syntax. There's some discussion about this here.

I promise I get zero joy out of making changes to other project's build configurations, I didn't make this change for the fun of it! 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, boo. That makes sense. In that case, I'm just going to suggest a small tweak to syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I'll just push that as a commit.

@adamsmasher adamsmasher requested a review from vreynolds January 18, 2023 19:37
@vreynolds vreynolds added type: bug version: bump patch A PR with release-worthy changes and is backwards-compatible. and removed status: oncall Flagged for awareness from Honeycomb Telemetry Oncall labels Jan 18, 2023
@vreynolds vreynolds merged commit a1ec0e6 into honeycombio:main Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug version: bump patch A PR with release-worthy changes and is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants