-
Notifications
You must be signed in to change notification settings - Fork 421
chore: Record subscriber usage metric #3626
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
Conversation
ab81e9e to
2ea9297
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3626 +/- ##
===========================================
- Coverage 97.84% 81.12% -16.72%
===========================================
Files 440 429 -11
Lines 57644 56363 -1281
Branches 1 1
===========================================
- Hits 56404 45727 -10677
- Misses 1240 10636 +9396
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e6f19e0 to
a76cd7f
Compare
bizob2828
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a coupe questions/comments. You're going to also have to remove the tracking of OnRequire metrics for subscribers. This is done here. Also you'll have a bunch of failing versioned tests as we have 1 per library to assert the previous OnRequire metrics, those will need to be updated and in all cases, rewritten as the previous metrics got incremented when a package was required. Now they aren't incremented until a method on the package is used
| * | ||
| * @returns {string} The version string from the package manifest or "unknown". | ||
| */ | ||
| function resolveModuleVersion(moduleSpecifier, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a very similar function in lib/util/get-package-version.js. you could pass off to use that for consistency after you get the base dir. If not, I'd still recommend setting the version to process.version if package.json cannot be found for consistency with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a rather big difference between the implementations. The function in this PR:
- Starts from the directory that the module specifier resolves to. So for
require('foo'), it'll start at/path/to/app/node_modules/foo/. - If a manifest cannot be found there, it traverses up the tree one directory
- Repeat looking for a manifest
- Repeat until we hit the app root / do not find a manifest
The function in lib/util/ simply looks in the provided directory and returns the Node.js version if a manifest is not found there.
Do you think we will ever have a subscriber that targets a specifier like moduleName/sub/module.js that would mean we need the tree traversal algorithm in this PR's version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bizob2828 this implementation has changed a bit. Please re-review it.
edbaf16 to
6daf011
Compare
fb3097d to
b3cba72
Compare
b3cba72 to
1beea6b
Compare
bizob2828
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very close. a few comments. Nice creativity on trying to obtain the appropriate path of the module
47968c1 to
95ed4c4
Compare
95ed4c4 to
0a3c636
Compare
bizob2828
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work on this one 🚢 🇮🇹 . Also, don't forget to update angler with all the lovely updates for this one
This PR resolves #3575.