-
Notifications
You must be signed in to change notification settings - Fork 27.7k
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
Warn, don’t crash, when running Next.js inside an IFrame #5523
Conversation
@@ -224,6 +224,9 @@ export default class Router { | |||
} | |||
|
|||
if (method !== 'pushState' || getURL() !== as) { | |||
if (window.frameElement) { |
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.
We don't do this check right 🤔
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.
So it probably should check for the method not being available
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.
@timneutkens The method IS available (I think), just restricted by the browser’s security model.
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.
@timneutkens Actually we don't need history to be rewritten if we run Next inside the iframe.
Isn't this already handled in this if branch: https://github.com/zeit/next.js/blob/8aecb8d061bc8c50dba3f4601296e9785a7646db/packages/next-server/lib/router/router.js#L215-L224 |
I see now that the problem only applyies to Friendly IFrame's without |
Oh, it looks like @hankmander's old PR #3437 is on the merge queue for 7.0.3: |
It is not, it was replaced by the window.history check. |
Could you fix the merge conflicts and add an integration test? I guess this is the only way to solve the specific issue with iframes. |
I'll close this for now since it's conflicting with the current codebase. |
Checks if Next.js is running inside an IFrame, in which case it doesn’t use
window.history
and throws a warning once.Previously, this would generate this error:
This is an update to #3437 (which is a solution for #3118).