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(htmlhint.ts): replace deprecated request module with what-wg fetch #670

Merged
merged 4 commits into from
Sep 16, 2021

Conversation

darcyparker
Copy link
Contributor

@darcyparker darcyparker commented Aug 24, 2021

Short description of what this resolves:
Replaces the deprecated request module with fetch.

Today, npm install warns:

request@2.88.0: request has been deprecated, see https://github.com/request/request/issues/3142
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.

(uuid is a dependency of request)

Proposed changes:

  • Remove request and @types/request
  • Replace with fetch()

Although fetch has a more modern async interface, I did not change the interface of htmlUrl(). htmlUrl() and other fns could be reworked to use more modern async patterns in the future. My main objective is to replace the deprecated request API.

@github-actions github-actions bot added cli Relates to HTMLHint's command-line interface dependencies Pull requests that update a dependency file labels Aug 24, 2021
@stuartbale
Copy link

Please link this pull request to the issue raised here:
#645

@stuartbale
Copy link

@darcyparker - this fix appears to have stalled, i think because by the time it was reviewed there are conflicts. I wonder if you could spare the time to resolve the conflicts and do another pull request to get some movement on this?
Regards,
Stuart

@darcyparker darcyparker force-pushed the replaceDeprecratedRequest branch 3 times, most recently from 09b5fe4 to 581815c Compare September 15, 2021 13:21
@darcyparker
Copy link
Contributor Author

@stuartbale - Yes, at the time of review, the package-lock.json had changed... I just rebased.

@coliff
Copy link
Member

coliff commented Sep 15, 2021

build/test is failing - can you take a look @thedaviddias @darcyparker ?

@darcyparker
Copy link
Contributor Author

@coliff - I am looking at it now.

@darcyparker
Copy link
Contributor Author

The build/test issue was node-fetch v3. Because this repo builds commonjs, I needed to use v2. (https://github.com/node-fetch/node-fetch#loading-and-configuring-the-module) v3 is only 15 days old (as of now)... so I missed this when I rebased and reinstalled node-fetch. I believe the tests will pass now.

@darcyparker
Copy link
Contributor Author

FYI: I added node-fetch types for v2 as dev dependency.

@thedaviddias
Copy link
Member

FYI: I added node-fetch types for v2 as dev dependency.

Thanks for doing that work @darcyparker 🙏

@thedaviddias thedaviddias merged commit 250169d into htmlhint:master Sep 16, 2021
github-actions bot pushed a commit that referenced this pull request Sep 16, 2021
## [0.15.2](v0.15.1...v0.15.2) (2021-09-16)

### Bug Fixes

* **htmlhint.ts:** replace deprecated request module with what-wg fetch ([#670](#670)) ([250169d](250169d))
@darcyparker darcyparker deleted the replaceDeprecratedRequest branch September 16, 2021 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Relates to HTMLHint's command-line interface dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants