-
Notifications
You must be signed in to change notification settings - Fork 97
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: clean up unused dependencies and env variables #471
Conversation
Setting 'null' to an empty string '' (which is falsy and the right thing here)
- Removing a number of packages from dependencies that simply were not in use. - Removing 'font-awesome' dependency which we didn't actually need in our stylesheet - we don't use it, though we were importing it and setting a variable from it. Not sure why it was ever there.
Codecov Report
@@ Coverage Diff @@
## master #471 +/- ##
=======================================
Coverage 60.30% 60.30%
=======================================
Files 44 44
Lines 776 776
Branches 132 132
=======================================
Hits 468 468
Misses 298 298
Partials 10 10 Continue to review full report at Codecov.
|
We get 'history' through frontend-platform.
@@ -12,26 +12,13 @@ LOGOUT_URL='http://localhost:18000/logout' | |||
MARKETING_SITE_BASE_URL='http://localhost:18000' | |||
ORDER_HISTORY_URL='localhost:1996/orders' | |||
REFRESH_ACCESS_TOKEN_ENDPOINT='http://localhost:18000/login_refresh' | |||
SEGMENT_KEY=null |
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.
is this because 'null' is treated as actually a truthy value? we have had this issue in our other repoes too where we had to use '' vs anything else for False, and True respectively
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.
Yes. null gets cast to a string and becomes truthy. Same with false. An empty string is falsy, though, when it hits JS so makes a much better default.
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.
lgtm since you verified none of the vars are used!
This repo had a number of old environment variables that were no longer in use. It also had a number of dependencies that weren't actually used in code anywhere.
The only one that was used was font-awesome, but it was only 'used' to the extent that it was imported into the stylesheet and then... not used. They're all gone.