SalesforceCommerceCloud / sfcc-ci

Salesforce Commerce Cloud CLI
https://npmjs.com/package/sfcc-ci
BSD 3-Clause "New" or "Revised" License
231 stars 94 forks source link

Replace deprecated request package with alternative #135

Open tobiaslohr opened 4 years ago

tobiaslohr commented 4 years ago

NPM package request has been deprecated. We should work to look out for suitable alternatives and start exploring them. One candidate might be got (https://www.npmjs.com/package/got), however alternative options are welcome.

clavery commented 2 years ago

got (like all of Sindre's packages nowadays) is ESM only in the current version. Given that sfcc-ci is commonjs at the moment that adds some friction since you can't do a simple require or you need to use a version < than the current.

I find axios (https://www.npmjs.com/package/axios) works fine for OCAPI, webdav (i.e. for both text and binary content). Still works fine with commonjs and is promise-native. So if we ever want to drop support for node<12 (which itself is EOL in April anyway) we could use it async as well. I use it here for ocapi/webdav and AM: https://github.com/SalesforceCommerceCloud/b2c-tools/blob/main/lib/environment.js

nickpalmigiano commented 2 years ago

axios seems like a more straightforward replacement since it keeps the same architecture patterns. While I like the fact that sfcc-ci follows similar design patterns as the storefront architecture, with pwa-kit this is less important. Since sfcc-ci is used by many different environment combinations I would consider node version compatibility important. Either got or axios would work since they seem to be the most well maintained, just have to understand the trade-offs.

gokaygurcan commented 1 year ago

https://npmtrends.com/axios-vs-fetch-vs-got-vs-ky-vs-request-vs-simple-get-vs-superagent

Looks like Axios is very reliable alternative.

Currently, there're 3 warnings, and they're all related to request package:

npm WARN deprecated uuid@3.4.0: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
npm WARN deprecated har-validator@5.1.5: this library is no longer supported
npm WARN deprecated request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142

@tobiaslohr do you need any support on this?

tobiaslohr commented 1 year ago

@gokaygurcan Yes, I do! Happy to get support for this. I think there are a few candidates, also see this thread https://github.com/request/request/issues/3143.

While there are personal preferences I think we should ensure that the replacement accounts at least for the following principles:

Thoughts?

DenyVeyten commented 5 months ago

Facing more challenges due to request package here: can't easily use https://github.com/mswjs/interceptors to mock responses for requests sfcc-ci sends to OCAPI (for integration testing purposes) because of strictSSL: true (rejectUnauthorized: true).

Current workaround is to use "self-signed": true in dw.json, otherwise requests throws TypeError: Cannot read properties of null (reading 'authorized') from https://github.com/request/request/blob/3c0cddc7c8eb60b470e9519da85896ed7ee0081e/request.js#L945

It may be an issue with MSW itself as well, but indeed request should be replaced first.