Throw error if getInitialProps is defined as as instance method #4922
Conversation
HaNdTriX
force-pushed
the
get-initial-props
branch
from
be3cd41
to
80bfed7
Compare
lib/utils.js
Outdated
| @@ -54,6 +54,15 @@ export function isResSent (res) { | |||
| } | |||
|
|
|||
| export async function loadGetInitialProps (Component, ctx) { | |||
| if ( | |||
| process.env.NODE_ENV !== 'production' && | |||
There was a problem hiding this comment.
This should actually be a completely separate statement:
if(process.env.NODE_ENV !== 'production') {
if(Component.prototype && Component.prototype.getInitialProps) {
// etc
}
}
This makes sure uglify removes it in production.
There was a problem hiding this comment.
Good point. Updated the PR!
HaNdTriX
force-pushed
the
get-initial-props
branch
2 times, most recently
from
7e49218
to
150b892
Compare
lib/utils.js
Outdated
| if (process.env.NODE_ENV !== 'production') { | ||
| if (Component.prototype && Component.prototype.getInitialProps) { | ||
| const compName = getDisplayName(Component) | ||
| console.warn(`"${compName}.getInitialProps()" is defined as an instance method.`) |
There was a problem hiding this comment.
I actually wonder if we should throw here, there's no good use case for having getInitialProps as an instance method on a top-level component, as it will confuse both yourself and team members. Also by throwing we make sure react-error-overlay is rendered
There was a problem hiding this comment.
If we throw here, this will be a major change. So we'll have to wait for next.js Version 7
There was a problem hiding this comment.
Doesn't matter, canary already has webpack 4 so it's going to be a major anyway 👍
There was a problem hiding this comment.
Oh forgot about production statement. So I am fine with throwing. 😀
There was a problem hiding this comment.
Alright I'll change it. Should we create an error document as well? https://github.com/zeit/next.js/tree/canary/errors
There was a problem hiding this comment.
Alright I just fixed a few typos 😬. Now I hope everything is ok. Thanks a lot for reviewing!
HaNdTriX
force-pushed
the
get-initial-props
branch
4 times, most recently
from
7133985
to
5741e75
Compare
HaNdTriX
changed the title
|
Since we’re throwing now, can you add a test for it 👍🏻🙏🏻 |
HaNdTriX
force-pushed
the
get-initial-props
branch
from
1780a6c
to
dc8e14d
Compare
| } | ||
| const rejectPromise = loadGetInitialProps(TestComponent, {}) | ||
| const error = new Error('"TestComponent.getInitialProps()" is defined as an instance method - visit https://err.sh/next.js/get-inital-props-as-an-instance-method for more information.') | ||
| return expect(rejectPromise).rejects.toEqual(error) |
There was a problem hiding this comment.
Please note: There is a more elegant way to test for rejected promises in jest, but this is not working with our current jest version.
Omitting the static keyword happens pretty often. Therefore we should trigger a warning in devmode. Closes: vercel#4782
HaNdTriX
force-pushed
the
get-initial-props
branch
from
dc8e14d
to
7a0b3d7
Compare
|
Looks great! Thanks @HaNdTriX 🙏 |
Omitting the static keyword happens pretty often. Therefore we should trigger a warning in devmode.
Closes: #4782