Fix modsn.us URL shortening script #3212
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). nusmods-website – ./website🔍 Inspect: https://vercel.com/nusmodifications/nusmods-website/k3tidkwwk nusmods-export – ./export🔍 Inspect: https://vercel.com/nusmodifications/nusmods-export/h6p88dvz7 |
Codecov Report
@@ Coverage Diff @@
## master #3212 +/- ##
==========================================
- Coverage 55.44% 55.37% -0.07%
==========================================
Files 254 255 +1
Lines 5317 5320 +3
Branches 1227 1228 +1
==========================================
- Hits 2948 2946 -2
- Misses 2369 2374 +5
Continue to review full report at Codecov.
|
|
Deployment preview for |
|
Curious about this living under Also I can't believe I've not noticed the shorturl endpoint is literally just a proxy to |
|
Unfortunately no. By default, Vercel expects serverless functions to be placed under an /api directory at the root of the project, and the Vercel nusmods-website project is rooted at /website. I think we could try to configure that, but iirc support for that was a little sketchy, and I'm pretty sure it won't accept functions located outside the project root. Alternatively, we could create a new nusmods-serverless Vercel project rooted at a new /serverless directory. We'll just need to host it on a different domain, e.g. maybe serverless.nusmods.com or similar. I'm not sure if there's any benefit to doing that though.
Yeah ikr, I realized that when working on Dockerizing it. It also seems pretty dumb because that script/endpoint is publicly accessible, so it basically allows anyone to shorten a URL with modsn.us 🤷 I'm actually surprised nobody has abused it yet. |
|
@nusmodifications/nusmods-developers This is ready for review. TL;DR I've fixed but disabled URL shortening as shortening is a bit too slow. But we need to merge this asap to unblock the MPE work (#3035) |
| @@ -125,6 +126,7 @@ | |||
| "@material/fab": "0.40.1", | |||
| "@material/snackbar": "0.40.1", | |||
| "@sentry/browser": "5.30.0", | |||
| "@sentry/node": "6.0.2", | |||
There was a problem hiding this comment.
This uses a different major version from our website lol
There was a problem hiding this comment.
Yes... yes it does 😆 I didn't want the overhead of upgrading packages at this time but looks like v6 doesn't actually change any APIs. I'll bump all @sentry/* before merging.
| * - https://github.com/nusmodifications/nusmods/issues/2057 | ||
| * - https://github.com/nusmodifications/nusmods/pull/3212 | ||
| */ | ||
| export const enableShortUrl = false; |
There was a problem hiding this comment.
Some way to override this at runtime would be nice, maybe reuse the debug flags code by reading them from query params
There was a problem hiding this comment.
I think we should actually move all the debug.ts stuff into dynamic feature flags that check query params on module init, which will prevent pages that change the params (e.g. Venues, Module search) from flipping the debug flags. This can also replace useGlobalDebugValue, with the downside of losing the ability to flip flags after our JS loads.
I'll move this into another issue.
Context
Implements a fix for #2057 behind a new
enableShortUrlfeature flag.NOTE: This PR doesn't actually fix the issue for end users as the flag is disabled. See the TODO below for more info.
This PR reimplements the 6-year-old short_url.php in a Vercel serverless function using TypeScript. short_url.php had been lying unused since around the time we moved to a Dockerized setup (in #1937). The Dockerized setup was abandoned recently (#3098), and gives us the opportunity to deploy this in a serverless manner.
Also lays groundwork for adding more serverless functions to /website, required by #3035.
TODO: because the serverless function imposes a huge delay on URL shortening (it takes 2-3s), we'll want to display the unshortened URL immediately while the shortener loads. We can alternatively make it shorten on demand (i.e. a shorten button) or optionally (i.e. have a toggle). (EDIT: as this requires some UI work and iteration, we'll punt this to a future PR to unblock the urgent-ish #3035)
Implementation
In the future, we could investigate implementing our own URL shortener using Cloudflare Workers, or folding URL shortening into the new GraphQL server (#1726). YOURLS seems overcomplicated for what's essentially just a persisted dictionary.