-
Notifications
You must be signed in to change notification settings - Fork 309
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
Don't use deprecated url.parse function #4743
Don't use deprecated url.parse function #4743
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
a2bb725
to
e5192c8
Compare
Overall package sizeSelf size: 7.21 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.1.1 | 18.67 MB | 18.68 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.4.1 | 2.14 MB | 2.23 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | jsonpath-plus | 9.0.0 | 580.4 kB | 1.03 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | lru-cache | 7.14.0 | 74.95 kB | 74.95 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4743 +/- ##
==========================================
- Coverage 91.42% 88.00% -3.43%
==========================================
Files 112 301 +189
Lines 3475 12744 +9269
Branches 33 33
==========================================
+ Hits 3177 11215 +8038
- Misses 298 1529 +1231
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e5192c8
to
eb7dc82
Compare
BenchmarksBenchmark execution time: 2024-10-02 07:19:53 Comparing candidate commit 8693eb3 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 257 metrics, 9 unstable metrics. |
eb7dc82
to
d845dd8
Compare
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.
thanks for this!
The problem I see with this is that we only fix the unit test, but a customer using this version of cypress would still crash.
Could we do a polyfill, like we do in packages/dd-trace/src/iitm.js
? Where we check the version of node we're running and do something like:
// url-polyfill.js
if (semver.satisfies(process.versions.node, '$NODE_VERSION')) {
module.exports = require('url')
} else {
const url = require('url')
url.urlToHttpOptions = ... // your polyfill
return url
}
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.
thanks!
The old code had several bugs/issues:
url.urlToHttpOptions
was a function had a bug (asurlToHttpOptions
was never imported) so the check would always returnfalse
.url.urlToHttpOptions
wasn't present didn't load the polyfill, but instead the deprecatedurl.parse
, so the polyfill was actually never used in that case.url.urlToHttpOptions
was pressent was performed on every request instead of at load-time.unix:
from the URL when using named pipes on Windows.Note to reviewers
The minimum Node.js version supported by this tracer now ships with
url.urlToHttpOptions
, so from that point of view, the polyfill is no longer needed. However, we still maintain support for an old version of Cypress that ships with an older built-in version of Node.js that doesn't haveurl.urlToHttpOptions
. So for that reason, we need to still have a polyfill.