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

Replace deprecated request package with Fetch API #2858

Merged
merged 1 commit into from Sep 16, 2022

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Sep 16, 2022

I've split this change from #2850 so it's easier to review

The changes resolve 3x vulnerabilities in node-sass and removes this message on install:

npm WARN deprecated request@2.88.2: request has been deprecated, see request/request#3142

We'll see to check for CSS changes from node-sass@7.01node-sass@7.03

I've replaced request() with Fetch API following this approach (until native support arrives in Node.js 17):
https://blog.logrocket.com/fetch-api-node-js/#undici

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2858 September 16, 2022 07:59 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2858 September 16, 2022 08:00 Inactive
@colinrotherham colinrotherham added this to Needs review 🔍 in Design System Sprint Board Sep 16, 2022
@colinrotherham colinrotherham changed the title Remove deprecated request package Replace deprecated request package with Fetch API Sep 16, 2022
@colinrotherham colinrotherham self-assigned this Sep 16, 2022
@36degrees
Copy link
Member

Just to check, is there any connection between removing request and bumping node sass? Or is it just that they both alter the lock file so they're lumped in together?

@colinrotherham
Copy link
Contributor Author

Just to check, is there any connection between removing request and bumping node sass? Or is it just that they both alter the lock file so they're lumped in together?

@36degrees Yeah it's a child dependency that they recently removed:

govuk-frontend-repository@ /path/to/project/govuk-frontend
├─┬ node-sass@7.0.1
│ └── request@2.88.2 deduped
└── request@2.88.2

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2858 September 16, 2022 08:21 Inactive
@colinrotherham
Copy link
Contributor Author

colinrotherham commented Sep 16, 2022

Rebased with #2857 and rebuilt package-lock.json

@36degrees
Copy link
Member

36degrees commented Sep 16, 2022

Yeah it's a child dependency that they recently removed

Just to flag as this confused me – although request has been removed in node-sass' master branch it looks like v7.0.3 was released from a hotfix branch that is currently 17 commits behind master and still includes request.

➜ npm ls request                   
govuk-frontend-repository@ /Users/oliver.byford/Code/govuk-frontend
└─┬ node-sass@7.0.3
  └── request@2.88.2

It looks like this is because 7.0.2 broke compatibility with Node 16 and so they yanked the release and released 7.0.3 as a hotfix.

@36degrees
Copy link
Member

Looks like #2856 also bumps node-sass – do you have a preference re either merging that and rebasing this, or closing Dependabot's PR in favour of this one?

@colinrotherham
Copy link
Contributor Author

@36degrees I'm happy with this PR, and let the Dependabot one auto-close. Thanks for checking

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2858 September 16, 2022 10:10 Inactive
@colinrotherham
Copy link
Contributor Author

@36degrees Good spot on the hot fix branch though

I've updated my commit message to say "Begin removing"

If you can review? Thanks

Copy link
Member

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

I haven't used undici before – from a quick look at their docs it looks like there's some best practise for using it in tests which suggest reducing the keep alive timeouts, which we may want to consider.

Otherwise I'm happy with these changes 👍🏻

@36degrees
Copy link
Member

Actually, just a thought – given the node-sass change is now unrelated is it worth backing that change out? Dependabot has a PR open for it anyway which we can use and it should automatically rebase 🤷🏻

@colinrotherham
Copy link
Contributor Author

@36degrees Yeah course, your call. I'll rebase after

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2858 September 16, 2022 11:32 Inactive
}
})
// Reduce test keep-alive
setGlobalDispatcher(new Agent({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@36degrees Thanks for the link on this bit, hopefully keeps the test runs short and sweet

Ready to review again now

@colinrotherham colinrotherham merged commit a1a1944 into main Sep 16, 2022
Design System Sprint Board automation moved this from Needs review 🔍 to Done 🏁 Sep 16, 2022
@colinrotherham colinrotherham deleted the remove-deprecated-request-more branch September 16, 2022 11:49
@colinrotherham colinrotherham removed their assignment Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants