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.

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

Documentation about Head.rewind #9326

Closed
lcswillems opened this issue Nov 6, 2019 · 12 comments · Fixed by #14091
Closed

Documentation about Head.rewind #9326

lcswillems opened this issue Nov 6, 2019 · 12 comments · Fixed by #14091
Assignees
Milestone

Comments

@lcswillems
Copy link

lcswillems commented Nov 6, 2019

Feature request

Is your feature request related to a problem? Please describe.

I see Head.rewind() being used in several examples (e.g. the apollo example) but I don't really understand what it does and can't find documentation about it.

Describe the solution you'd like

If possible, I would like some documentation about it. What happens if I omit it? Could it be possible to have a concrete example?

@timneutkens
Copy link
Member

We can get rid of .rewind by now as the latest next/head no longer uses legacy context. I'll open up a PR soon.

@timneutkens timneutkens self-assigned this Nov 6, 2019
@Timer Timer added this to the 9.1.x milestone Nov 9, 2019
@iFallUpHill
Copy link

@timneutkens Could you explain what implications this has from a refactoring perspective?
Am I safe to simply not call Head.rewind() if I'm on a 9.1.x release?

@zuqini
Copy link

zuqini commented Nov 27, 2019

@timneutkens In addition, could you clarify what you mean by legacy context? Is there any other aspects of the code that we need to refactor with this change?

I also see that next/head will be deprecated in the near future. What implications will that have? Thanks!

@dminkovsky
Copy link

dminkovsky commented Dec 28, 2019

Very new to Next. My guess based on this thread, the name of the method, and examples I've seen: It's probably got to do with how Head is populated as a side-effect of rendering. In the examples with Head.rewind I've seen, the method is called after rendering that is meant to compute properties for rendering. I'm guessing this undoes changes to Head caused during such renders so it can be populated later.

@Timer Timer modified the milestones: 9.1.x, 9.2.x Jan 3, 2020
@jaydenseric
Copy link
Contributor

jaydenseric commented Feb 25, 2020

@iFallUpHill I have done some digging, and it seems that next/head has not used legacy context since next@8.0.0-canary.7, here is the PR that was merged: #6038. Based on this comment above, it can be assumed that it’s safe to stop using Head.rewind() from that version onward.

jaydenseric added a commit to jaydenseric/next-graphql-react that referenced this issue Feb 25, 2020
This has apparently been unnecessary since next@8.0.0-canary.7: vercel/next.js#9326 (comment)
@timneutkens
Copy link
Member

@jaydenseric Head.rewind is currently still needed if you do tree rendering, eg getDataFromTree in Apollo

Would highly recommend not doing tree rendering as it has massive performance implications.

@jaydenseric
Copy link
Contributor

jaydenseric commented Feb 25, 2020

@timneutkens

Head.rewind is currently still needed if you do tree rendering, eg getDataFromTree in Apollo

This is surprising to hear, considering it was deliberately removed from the Next.js Apollo example:

1a50d99#diff-c7ee3d6e815d630e8d7be23ea73b6436L107

Are you sure it is necessary; is your earlier comment and the Apollo example now incorrect?

Would highly recommend not doing tree rendering as it has massive performance implications.

I know all about tree rendering, I've worked with the Apollo one and published before using several different techniques:

Are you able to offer an alternative for server side rendering nested query components?

@timneutkens
Copy link
Member

This is surprising to hear, considering it was deliberately removed from the Next.js example:

1a50d99#diff-c7ee3d6e815d630e8d7be23ea73b6436L107

Are you sure it is necessary; is your earlier comment and the Apollo example now incorrect?

Yes I'm pretty sure it's necessary, so yeah the example should be updated.

My comment is still correct, I said "We can get rid of it" haven't had the time to do so because of other priorities.

Are you able to offer an alternative for server side rendering nested query components?

Extracting the data requirements out of the React tree like Relay does and then executing it in getStaticProps/getServerProps (new data fetching methods)

jaydenseric added a commit to jaydenseric/next-graphql-react that referenced this issue Feb 26, 2020
This reverts commit 142f1a2.

Apparently Head.rewind() is necessary after all: vercel/next.js#9326 (comment)
timneutkens added a commit to timneutkens/next.js that referenced this issue Jun 11, 2020
@lfades
Copy link
Member

lfades commented Jun 11, 2020

Update: There aren't any examples using Head.rewind anymore and it will be removed soon it's now deprecated.

@timneutkens
Copy link
Member

It's not being removed btw, #14091 is backwards compatible

@kodiakhq kodiakhq bot closed this as completed in #14091 Jun 11, 2020
kodiakhq bot pushed a commit that referenced this issue Jun 11, 2020
@Timer Timer modified the milestones: 9.x.x, june 2020 Jun 11, 2020
darshkpatel pushed a commit to MLH-Fellowship/next.js that referenced this issue Jun 12, 2020
rokinsky pushed a commit to rokinsky/next.js that referenced this issue Jul 11, 2020
@JustFly1984
Copy link

JustFly1984 commented Aug 25, 2021

And now it is 2021, and I found one in the legacy code, while updating and refactoring. Just removing it from apollo.tsx

typescript complains that rewind does not exist on Head, which has type ({ children }: { children: ReactNode; }) => Element

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants