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

chore: remove express-device #2498

Merged
merged 1 commit into from
Aug 2, 2021
Merged

chore: remove express-device #2498

merged 1 commit into from
Aug 2, 2021

Conversation

timotheeg
Copy link
Contributor

@timotheeg timotheeg commented Aug 2, 2021

Context

We have a vulnerability report on express-device here

express-device has not been upgraded for some time, but diving deeper in code, it looks like we do not actually use the capabilities provided by express-device ? 🤔

The following command yields nothing:

git grep -E 'is_(desktop|phone|tablet|mobile|tv|bot|car)|device_(type|name)'

Approach

Remove express-device and all the references.

@mantariksh
Copy link
Contributor

we should probably check that the user agent string still appears as expected in the Cloudwatch logs

@timotheeg
Copy link
Contributor Author

timotheeg commented Aug 2, 2021

True. I would think that user-agent logging is a built-in functionality of express itself. express-device's job is to provide device type flags, and I see no code where we're using these flags or where we add them to log.

@timotheeg timotheeg force-pushed the remove_express_device branch from 9c3c5b1 to ac422f6 Compare August 2, 2021 06:51
@timotheeg timotheeg marked this pull request as ready for review August 2, 2021 07:46
@timotheeg
Copy link
Contributor Author

Tested on staging with User-Agent still showing in log 👍

If someone has extra context on why we were using express-device in the first place, and a reason we shouldn't remove it, please speak now :)

@mantariksh @karrui @tshuli

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

If someone has extra context on why we were using express-device in the first place, and a reason we shouldn't remove it, please speak now :)

¯_(ツ)_/¯ was before our time lol

@timotheeg
Copy link
Contributor Author

Right 😄 ! Well, I'm just going to merge then. @liangyuanruo if you remember something, let us know :)

@timotheeg timotheeg merged commit 8583336 into develop Aug 2, 2021
@timotheeg timotheeg deleted the remove_express_device branch August 2, 2021 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants