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.

Sign up for GitHub

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

Jump to bottom

Add better typing for redirect #15603

Merged
merged 1 commit into from
Jul 29, 2020
Merged

Add better typing for redirect #15603

merged 1 commit into from
Jul 29, 2020

Conversation

robertvansteen
Copy link
Contributor

@robertvansteen robertvansteen commented Jul 28, 2020 •
edited by Timer
Loading

Related: #15594.

This pull request adds a better typing for the redirect method on the NextApiResponse type.

Redirect was introduced in #14705 but the typing for the params was statusOrUrl: string | number, url?: string) which is too broad and allows for an invalid combination. I have added an overload to allow two possible combinations: url: string or status: number, url: string.

@ijjk
Copy link
Member

ijjk commented Jul 28, 2020

Stats from current PR

Default Server Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary rovansteen/next.js patch/15594 Change
buildDuration 11.8s 12s ⚠️ +209ms
nodeModulesSize 65.5 MB 65.5 MB ⚠️ +88 B
Page Load Tests Overall increase ✓
vercel/next.js canary rovansteen/next.js patch/15594 Change
/ failed reqs 0 0 ✓
/ total time (seconds) 2.145 2.125 -0.02
/ avg req/sec 1165.3 1176.24 +10.94
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.211 1.181 -0.03
/error-in-render avg req/sec 2064.96 2116.95 +51.99
Client Bundles (main, webpack, commons)
vercel/next.js canary rovansteen/next.js patch/15594 Change
677f882d2ed8..b7a9.js gzip 10.2 kB 10.2 kB ✓
framework.HASH.js gzip 39.1 kB 39.1 kB ✓
main-c5a676d..82de.js gzip 6.75 kB 6.75 kB ✓
webpack-488d..c0e7.js gzip 751 B 751 B ✓
Overall change 56.8 kB 56.8 kB ✓
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary rovansteen/next.js patch/15594 Change
677f882d2ed8..dule.js gzip 6.09 kB 6.09 kB ✓
framework.HA..dule.js gzip 39.1 kB 39.1 kB ✓
main-079bffd..dule.js gzip 5.83 kB 5.83 kB ✓
webpack-4f62..dule.js gzip 751 B 751 B ✓
Overall change 51.8 kB 51.8 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary rovansteen/next.js patch/15594 Change
polyfills-05..1236.js gzip 30.8 kB 30.8 kB ✓
Overall change 30.8 kB 30.8 kB ✓
Client Pages
vercel/next.js canary rovansteen/next.js patch/15594 Change
_app-8f5f611..1f7b.js gzip 1.28 kB 1.28 kB ✓
_error-a98d9..5cb7.js gzip 3.45 kB 3.45 kB ✓
hooks-f7f3d0..7465.js gzip 887 B 887 B ✓
index-08fb3f..c0e9.js gzip 227 B 227 B ✓
link-ddd176e..5566.js gzip 1.29 kB 1.29 kB ✓
routerDirect..8aa1.js gzip 284 B 284 B ✓
withRouter-f..e777.js gzip 284 B 284 B ✓
Overall change 7.71 kB 7.71 kB ✓
Client Pages Modern
vercel/next.js canary rovansteen/next.js patch/15594 Change
_app-669dbe5..dule.js gzip 626 B 626 B ✓
_error-d5979..dule.js gzip 2.3 kB 2.3 kB ✓
hooks-805c40..dule.js gzip 387 B 387 B ✓
index-6ba5a4..dule.js gzip 226 B 226 B ✓
link-69bc264..dule.js gzip 1.25 kB 1.25 kB ✓
routerDirect..dule.js gzip 284 B 284 B ✓
withRouter-d..dule.js gzip 282 B 282 B ✓
Overall change 5.36 kB 5.36 kB ✓
Client Build Manifests
vercel/next.js canary rovansteen/next.js patch/15594 Change
_buildManifest.js gzip 275 B 275 B ✓
_buildManife..dule.js gzip 281 B 281 B ✓
Overall change 556 B 556 B ✓
Rendered Page Sizes
vercel/next.js canary rovansteen/next.js patch/15594 Change
index.html gzip 947 B 947 B ✓
link.html gzip 951 B 951 B ✓
withRouter.html gzip 937 B 937 B ✓
Overall change 2.83 kB 2.83 kB ✓
Serverless Mode
General Overall increase ⚠️
vercel/next.js canary rovansteen/next.js patch/15594 Change
buildDuration 13.2s 13.1s -113ms
nodeModulesSize 65.5 MB 65.5 MB ⚠️ +88 B
Client Bundles (main, webpack, commons)
vercel/next.js canary rovansteen/next.js patch/15594 Change
677f882d2ed8..b7a9.js gzip 10.2 kB 10.2 kB ✓
framework.HASH.js gzip 39.1 kB 39.1 kB ✓
main-c5a676d..82de.js gzip 6.75 kB 6.75 kB ✓
webpack-488d..c0e7.js gzip 751 B 751 B ✓
Overall change 56.8 kB 56.8 kB ✓
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary rovansteen/next.js patch/15594 Change
677f882d2ed8..dule.js gzip 6.09 kB 6.09 kB ✓
framework.HA..dule.js gzip 39.1 kB 39.1 kB ✓
main-079bffd..dule.js gzip 5.83 kB 5.83 kB ✓
webpack-4f62..dule.js gzip 751 B 751 B ✓
Overall change 51.8 kB 51.8 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary rovansteen/next.js patch/15594 Change
polyfills-05..1236.js gzip 30.8 kB 30.8 kB ✓
Overall change 30.8 kB 30.8 kB ✓
Client Pages
vercel/next.js canary rovansteen/next.js patch/15594 Change
_app-8f5f611..1f7b.js gzip 1.28 kB 1.28 kB ✓
_error-a98d9..5cb7.js gzip 3.45 kB 3.45 kB ✓
hooks-f7f3d0..7465.js gzip 887 B 887 B ✓
index-08fb3f..c0e9.js gzip 227 B 227 B ✓
link-ddd176e..5566.js gzip 1.29 kB 1.29 kB ✓
routerDirect..8aa1.js gzip 284 B 284 B ✓
withRouter-f..e777.js gzip 284 B 284 B ✓
Overall change 7.71 kB 7.71 kB ✓
Client Pages Modern
vercel/next.js canary rovansteen/next.js patch/15594 Change
_app-669dbe5..dule.js gzip 626 B 626 B ✓
_error-d5979..dule.js gzip 2.3 kB 2.3 kB ✓
hooks-805c40..dule.js gzip 387 B 387 B ✓
index-6ba5a4..dule.js gzip 226 B 226 B ✓
link-69bc264..dule.js gzip 1.25 kB 1.25 kB ✓
routerDirect..dule.js gzip 284 B 284 B ✓
withRouter-d..dule.js gzip 282 B 282 B ✓
Overall change 5.36 kB 5.36 kB ✓
Client Build Manifests
vercel/next.js canary rovansteen/next.js patch/15594 Change
_buildManifest.js gzip 275 B 275 B ✓
_buildManife..dule.js gzip 281 B 281 B ✓
Overall change 556 B 556 B ✓
Serverless bundles
vercel/next.js canary rovansteen/next.js patch/15594 Change
_error.js 1.02 MB 1.02 MB ✓
404.html 4.18 kB 4.18 kB ✓
hooks.html 3.82 kB 3.82 kB ✓
index.js 1.02 MB 1.02 MB ✓
link.js 1.06 MB 1.06 MB ✓
routerDirect.js 1.05 MB 1.05 MB ✓
withRouter.js 1.05 MB 1.05 MB ✓
Overall change 5.2 MB 5.2 MB ✓
Commit: 3759d39

styfle
styfle reviewed Jul 28, 2020
View reviewed changes
Copy link
Member

@styfle styfle left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This fixes the TS types but what about if the user is using JS?

They could still do the following and get the same error Invalid value "undefined" for header "Location".

module.exports = (req, res) => res.redirect(307);

@timneutkens Do we need to throw a better error in this case?

@markozxuu
Copy link
Contributor

This fixes the TS types but what about if the user is using JS?

They could still do the following and get the same error Invalid value "undefined" for header "Location".

module.exports = (req, res) => res.redirect(307);

@timneutkens Do we need to throw a better error in this case?

This fixes the TS types but what about if the user is using JS?

They could still do the following and get the same error Invalid value "undefined" for header "Location".

module.exports = (req, res) => res.redirect(307);

@timneutkens Do we need to throw a better error in this case?

This should work in JS as TS and most importantly don't break previous versions with this change

Timer
Timer approved these changes Jul 29, 2020
View reviewed changes
Copy link
Member

@Timer Timer left a comment

Choose a reason for hiding this comment

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

This LGTM to land. I removed that it resolves the referenced issue, since we still want to handle this case gracefully with a proper error message for non-TS users.
Thanks!

@robertvansteen
Copy link
Contributor Author

Makes sense. I see some tests that are failing but they don't appear to be related to typing. Are there any issues with these tests?

@timneutkens
Copy link
Member

Are there any issues with these tests?

Windows tests flake sometimes, your PR is fine 👍

@timneutkens timneutkens merged commit 6eea915 into vercel:canary Jul 29, 2020
@robertvansteen robertvansteen deleted the patch/15594 branch July 29, 2020 07:07
@markozxuu markozxuu mentioned this pull request Aug 3, 2020
Merged
kodiakhq bot pushed a commit that referenced this pull request Aug 6, 2020
@markozxuu
Added Friendly error for res.redirect 🐝 (#15844)
6375026 
## Which solves this PR

 Displaying a friendly error, when the user is only passing `statusOrUrl`(type number) and the second argument `url` is ignored.

**Example**

`res.redirect(307);` // Show friendly error

Closes: #15594
x-ref: #15603
LauraBeatris pushed a commit to LauraBeatris/next.js that referenced this pull request Sep 1, 2020
@robertvansteen @LauraBeatris
Add better typing for redirect (vercel#15603)
a1ca9e3
@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@styfle styfle styfle left review comments

@timneutkens timneutkens timneutkens approved these changes

@Timer Timer Timer approved these changes

@ijjk ijjk Awaiting requested review from ijjk

@lfades lfades Awaiting requested review from lfades

Assignees
No one assigned
Labels
type: next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@robertvansteen @ijjk @markozxuu @timneutkens @styfle @Timer