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

refactor: use core Node.js resolve function if possible #25

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

watson
Copy link
Contributor

@watson watson commented Mar 20, 2019

I'm actually a little unsure about if this actually works 100% as expected. But if it does, I think this is a good improvement. See browserify/resolve#188 for details.

So I'm a bit hesitant to merge this until we know for sure.

@watson watson self-assigned this Mar 20, 2019
@watson watson requested a review from Qard March 20, 2019 15:33
Copy link
Contributor

@Qard Qard left a comment

Choose a reason for hiding this comment

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

Seems reasonable, as far as I can tell.

@trentm trentm added the agent-nodejs Make available for APM Agents project planning. label Nov 16, 2020
@timfish
Copy link

timfish commented Jan 17, 2025

Can we revisit this PR?

We've got reports that this impacts performance by around 30%

@watson
Copy link
Contributor Author

watson commented Jan 18, 2025

This was never merged because I never put in the time to get confident in it not breaking under certain circumstances (as mentioned in the PR description). I think my main concern was the feature detection logic. I see two roads forward right now:

  1. We ship it as is
  2. We bump the min required Node.js version to 8.9.0 or later, in which case we don't need to feature detection logic and can just use the Node.js core function

@timfish
Copy link

timfish commented Jan 19, 2025

It looks like this PR fails the tests for Node 8, 10, 12 and 14 but they pass for Node 16 onwards.

I'll try and work out why!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants