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

3.0 api #41

Closed
TrySound opened this issue Jul 14, 2018 · 15 comments
Closed

3.0 api #41

TrySound opened this issue Jul 14, 2018 · 15 comments
Labels
breaking breaking change

Comments

@TrySound
Copy link
Collaborator

TrySound commented Jul 14, 2018

We have a couple of breaking changes and I need some brainstorming before the next release.

  • HeadProvider has necessary headTags prop which is not necessary for client-only apps so users should specify it as empty arrays
  • HeadProvider is used to solve cascading tags
  • HeadTag is able to work without HeadProvider; will always render tag
  • cascading handling requires unchanged tag prop in HeadTag

Questions

  1. Should we provide default prop for headTags with empty array?
  2. Should we enforce having HeadProvider in the tree by throwing an error if context value is null?
  3. Should we throw and error in componentDidUpdate if tag prop is changed or just remove HeadTag from public api and provide more aliases?

/cc @tizmagik

@tizmagik
Copy link
Owner

tizmagik commented Jul 16, 2018

Should we provide default prop for headTags with empty array?

This makes sense, however it should warn/throw an error on the server if the prop is not provided in those cases since this means misconfigured server rendering. Maybe can do a typeof window check and default to empty array on client and throw when on the server?

Should we enforce having HeadProvider in the tree by throwing an error if context value is null?

The only valid use case for not having HeadProvider in the tree is for cases when a user doesn't care about the cascade, right? Which we can treat as a misconfiguration and then throwing an error would make sense. Any other alternatives? Would a console.error not be enough you think?

Should we throw and error in componentDidUpdate if tag prop is changed or just remove HeadTag from public api and provide more aliases?

What are the cases where the tag prop would change? Are there any valid uses cases for this? Since we already provide all of the W3C-compliant head attributes it may make sense to either remove the HeadTag from the public api or at least not publicize it in the documentation as much.

@tizmagik tizmagik added the breaking breaking change label Jul 16, 2018
TrySound added a commit that referenced this issue Aug 11, 2018
Ref #41

We provider all of the W3C-compliant head tags, so HeadTag is not
necessary as public api anymore.
TrySound added a commit that referenced this issue Aug 11, 2018
Ref #41

We provider all of the W3C-compliant head tags, so HeadTag is not
necessary as public api anymore.
TrySound added a commit that referenced this issue Aug 11, 2018
Ref #41

We provider all of the W3C-compliant head tags, so HeadTag is not
necessary as public api anymore.
TrySound added a commit that referenced this issue Aug 12, 2018
Ref #41

In this diff I made headTags optional and call invariant with ssr
check.

For invariant I used `tiny-invariant` package which is quite small and
simplified version of `fbjs/lib/invariant`.

`babel-plugin-dev-expression` wraps invariant with two cases
- throw error with message in development
- throw empty error in production `invariant failed` message

This allows to strip development only messages and save bytes in user
bundle.
tizmagik pushed a commit that referenced this issue Aug 13, 2018
Ref #41

In this diff I made headTags optional and call invariant with ssr
check.

For invariant I used `tiny-invariant` package which is quite small and
simplified version of `fbjs/lib/invariant`.

`babel-plugin-dev-expression` wraps invariant with two cases
- throw error with message in development
- throw empty error in production `invariant failed` message

This allows to strip development only messages and save bytes in user
bundle.
TrySound added a commit that referenced this issue Aug 13, 2018
Ref #41

In this diff I made HeadProvider always required to not confuse users
why title and meta doesn't work with cascade.
TrySound added a commit that referenced this issue Aug 13, 2018
Ref #41

In this diff I made HeadProvider always required to not confuse users
why title and meta doesn't work with cascade.
TrySound added a commit that referenced this issue Aug 13, 2018
Ref #41

In this diff I made HeadProvider always required to not confuse users
why title and meta doesn't work with cascade.
@TrySound
Copy link
Collaborator Author

@tizmagik Do we care about ie11 support? We use .remove() method which is not supported there.

@tizmagik
Copy link
Owner

Hmm yea good point, I personally don’t care about IE11 support, but if that’s the only thing that’s breaking support maybe we could address it? What do you think @TrySound ?

@TrySound
Copy link
Collaborator Author

forEach on NodeList too. Let's keep as is until somebody will request ie support.

@TrySound
Copy link
Collaborator Author

@tizmagik I think we are ready for release.

@tizmagik
Copy link
Owner

Awesome, great work @TrySound -- will cut a release as soon as I get a moment

@tizmagik
Copy link
Owner

Just a quick update here, a couple more things I want to get done before a 3.0 release

  • Update example app to support new API
  • Some sort of migration guide (maybe can be included with the release notes)

Will try and get to these ASAP, unless someone else wants to (feel free).

@tizmagik
Copy link
Owner

I was working through upgrading the example app and it struck me that <HeadProvider /> really feels like an implementation detail.

What do you think about renaming it to <ReactHead /> and making it the default export (as part of 3.0 release), e.g.

/* client.js */
import React from 'react';
import ReactHead, { Title, Link, Meta } from 'react-head';

const App = () => (
  <ReactHead>
    <div className="Home">
      <Title>Title of page</Title>
      <Link rel="canonical" content="http://jeremygayed.com/" />
      <Meta name="example" content="whatever" />
      // ...
    </div>
  </ReactHead>
);

@TrySound
Copy link
Collaborator Author

Nope.

  1. default exports makes distribution trickier. cjs output will have default export which should be handled by tools. And for example webpack is not able to bundle well cjs which imports esm with default export. In any case I also think default exports is a wrong part of the spec and makes module system too complex.
  2. Provider name is like a pattern. It's used in redux for years and used by react itself in context api. It's better to keep descriptive name HeadProvider.
  3. React in components names is overnaming. All exported components are react.

@TrySound
Copy link
Collaborator Author

The necessity of HeadProvider is defined by invariant. It should not be a problem for user.

@tizmagik
Copy link
Owner

Ok that makes sense, you convinced me to keep the name as-is 😁

I'm having a bit of trouble upgrading the example app but should hopefully get it going soon. I can cut another pre-release if you're itching to use the latest though. Lmk

@tizmagik
Copy link
Owner

tizmagik commented Aug 20, 2018

Ok example app updated. I think there's issues with react-router and the new React Context API or something, so I opted for @reach/router; #60

@TrySound
Copy link
Collaborator Author

Yes, this is known problem. Old context providers cannot be inside the new api provider.

@TrySound
Copy link
Collaborator Author

Hm, should be fixed in 16.4.
facebook/react#12551 (comment)

@tizmagik
Copy link
Owner

v3.0.0 has been released 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking breaking change
Projects
None yet
Development

No branches or pull requests

2 participants