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

'#21' - Add Identity-based idempotency support #668

Closed
wants to merge 4 commits into from

Conversation

timneutkens
Copy link
Member

This fixes #21.

Note:
Due to this reverse() https://github.com/zeit/next.js/blob/master/lib/head.js#L24 the last key is used instead of the first. I don't know if this is an issue.

Usage:

<Head>
  <link rel="stylesheet" href="/abc.css" key="test" />
  <link rel="stylesheet" href="/def.css" key="test" />
</Head>

Will only render <link rel="stylesheet" href="/def.css" key="test" />. When rendering new pages it checks if the key was already set. If so it will not remove the current key="test" and discard the new key="test". key is used by React for the same kind of functionality. So it seems the best possible prop to use. Though I'm open to changing it to id

}

this.unique.set(h.key, h)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is meant to cache the components that use key so that when switching routes it will set the cached component instead of the new one.

Example:
pages/index.js:

<Head>
   <link rel="stylesheet" href="/abc.css" key="test" />
</Head>

pages/other.js:

<Head>
   <link rel="stylesheet" href="/def.css" key="test" />
</Head>

if you navigate to / first and then to /other using a <Link> it will keep /abc.css instead of /def.css in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why we want to do this. I think we should use new one in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I might be better to remove this indeed. Since it will re-render anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nkzawa removed 👍

@nkzawa
Copy link
Contributor

nkzawa commented Jan 6, 2017

@rauchg Can you review this ?

@rauchg
Copy link
Member

rauchg commented Jan 8, 2017

  • Needs documenting in README section of <Head>
  • Does the usage of Set imply a polyfill?
    • If so, are we using Set elsewhere, so we've already paid its price?

@timneutkens
Copy link
Member Author

@rauchg On the second point:
It's used in quite a few places.
https://github.com/zeit/next.js/search?utf8=%E2%9C%93&q=new+Set

it says here babel-polyfill is needed:
https://babeljs.io/learn-es2015/#ecmascript-2015-features-map-set-weak-map-weak-set

We include https://babeljs.io/docs/plugins/transform-runtime/ in next.js. So It should be fine 👍

@timneutkens
Copy link
Member Author

@rauchg @nkzawa Added to readme 🙌

@rauchg rauchg added this to the 2.1 milestone Jan 10, 2017
@ehtb
Copy link

ehtb commented Jan 10, 2017

Just came across this issue: would it also be possible to use JSON.stringify({type, props}) as the key?

My concern is that in cases where there are a lot of meta tags (think social media directives), this could lead to errors which are difficult to trace / catch (duplicate keys used when they should not etc).

There is of course a performance penalty to be paid, not sure how large that is though.

@rauchg
Copy link
Member

rauchg commented Jan 10, 2017

The purpose of the key usage is for things that are meant to be deduped. For example, you could ensure certain components only inject some global <style> only once by specifying a key on it.

@rauchg
Copy link
Member

rauchg commented Jan 10, 2017

In other words: the vast majority of users will never specify a key on their <Head> children.

@rauchg
Copy link
Member

rauchg commented Apr 1, 2017

I actually haven't needed this in a while 🤔 Let's hold off on this until we feel the need again

@rauchg rauchg closed this Apr 1, 2017
@amankkg
Copy link

amankkg commented Apr 9, 2017

@rauchg this is surely needed by people who use non-css-in-js solution for components styling.
For now, we forced to explicitly import every css file in one place, say, layout component or _document.js file.
With this PR landed, I would place style content injection in component itself only.
Also, we can use simple wrapper to reduce boilerplate code:

const InjectStylesheet = ({ id, value }) => (
  <Header>
    <style key={id} dangerouslySetInnerHTML={{ __html: value }} />
  </Header>
)

and in someComponent.js:

import InjectStyleSheet from 'helpers/next/css'
import { classnames, stylesheet } from './style.css'

const SomeComponent = () => (
 <div className={classnames.root}>
    <InjectStyleSheet id="someComponent" value={stylesheet} />
    some content
  </div>
)

Obviously, for now this approach ends up with duplicate style tag injection every time the component is being used.

@akfish
Copy link

akfish commented Jun 30, 2017

Running into the exact same issue described above by @code4aman.
I ended up having to maintain a list of injected styles to kill the duplication.

@mschinis
Copy link

this would indeed be very useful, especially for managing meta tags

@lock
Copy link

lock bot commented May 10, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
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 this pull request may close these issues.

8 participants