Fix: Use local venue data for development instead of prod data #3810
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@kavishsathia is attempting to deploy a commit to the modsbot's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3810 +/- ##
==========================================
+ Coverage 53.87% 54.57% +0.69%
==========================================
Files 274 274
Lines 6032 6056 +24
Branches 1449 1449
==========================================
+ Hits 3250 3305 +55
+ Misses 2782 2751 -31 ☔ View full report in Codecov by Sentry. |
|
May I know what's the motivation behind changing this default? |
There was a problem hiding this comment.
Thank you for this contribution! Sorry it took me this long to get to it.
I like the idea of flipping the venue fetching such that it uses local by default and global in production.
Was there a specific reason you used a window.location.hostname check instead of the current environment string? If there wasn't, I'm thinking it might be better to use the existing config than the use the hostname.
| function isLocalhost() { | ||
| return window.location.hostname === 'localhost' || window.location.hostname === '127.0.0.1'; | ||
| } | ||
|
|
There was a problem hiding this comment.
Would it be make sense to use NUSMODS_ENV instead?
// Snippet from src/types/global.d.ts
/**
* NUSMods deployment environment.
*
* **Note:** although these environments may share the same name as
* `NODE_ENV` and other envs, there are subtle differences. Definitions:
*
* - `production`: a production deployment of NUSMods, i.e. a deployment meant to
* be used by the general public.
* - `staging`: a deployment of NUSMods's main development branch, which is
* not meant to be used by the general public but which may be promoted to
* production at any point.
* - `preview`: a deployment of work-in-progress branches, e.g. PR deploy
* previews.
* - `test`: we are in a test environment, e.g. a jest test run.
* - `development`: all other situations.
*/
declare const NUSMODS_ENV: 'development' | 'production' | 'staging' | 'preview' | 'test';| export function preferRepoVenues() { | ||
| return getParams().localVenue === '1'; | ||
| // Don't use repo data in jest tests | ||
| if (process.env.JEST_WORKER_ID !== undefined) return false; |
There was a problem hiding this comment.
Using NUSMODS_ENV would allow checking for NUSMODS_ENV === 'test' as well.
Context
Closes #3803
Implementation
The solution checks if the user is on localhost, and if they are, it will opt to use the local venue data instead of the production data by default. To override this behavior, the user can add a query parameter
prodVenueto their URL, which will enable them to use production data instead.Other Information