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

Invalid checksum #66

Closed
rtymchyk opened this issue Jun 26, 2017 · 31 comments
Closed

Invalid checksum #66

rtymchyk opened this issue Jun 26, 2017 · 31 comments
Assignees
Labels

Comments

@rtymchyk
Copy link

rtymchyk commented Jun 26, 2017

I am using the babel plugin 1.1.5 with styled components 2.1.0. I have placed a ssr: true in both the client and the server babel configs.

However, I always see this mismatch in checksum:

(client) P gradient-container___default-s1b1hs7t-
(server) P gradient-container-s1b1hs7t-0 ieOpth"

This tells me that the plugin is enabled for both the client and server, but the client consistently has this __default string as part of the class name. Where is this coming from?

@kitten
Copy link
Member

kitten commented Jun 26, 2017

Hm, can you try disabling the displayName option? It seems that something is leaking into the name there. Maybe the transpilation is not identical, and it's using default from export default or sth?

Also, currently the plugin doesn't support withComponent and extend. This will be added with the changes in v3

@rtymchyk
Copy link
Author

Thanks for your reply @philpl I don't use those methods from the API, but you have me an idea to try. Will get back to you.

@rtymchyk
Copy link
Author

rtymchyk commented Jul 18, 2017

So the issue was that we have components that look like

export default styled.div`
...
`

For now I'm just assigning it to a const and exporting that instead.

@kitten
Copy link
Member

kitten commented Jul 18, 2017

Ah, it seems then that we're not falling back to attaching a componentId based on the filename only. Something to look into, I suppose

On the other hand, generally it's not a good idea to directly export unnamed components. Exporting arrow functions without a name for example will lead to nameless components

But anyway, I'll have a quick look and see if there's any obvious problems in the plugin

@gregberge
Copy link

gregberge commented Jul 25, 2017

Also, currently the plugin doesn't support withComponent and extend. This will be added with the changes in v3

@philpl I need this, can I help?

@kitten
Copy link
Member

kitten commented Jul 25, 2017

@neoziro It's not possible to make this work with our current set-up of withConfig. It's going to change completely in v3 with a new architecture and core library for preprocessing, so we won't be tackling this right now.

If you need a workaround, you should be able to set your own component id for now:

const Button = styled.button`
  // ...
`

const Link = Button.withComponent('a').extend`
  // ...
`

Link.styledComponentId = `${Button.styledComponentId}_Link`;

This might work, but I haven't tried it yet.

Actually, this won't work because we need to use the ID to create a ComponentStyle instance.

But this makes me think. We should be able to add a workaround in SC right now:

Just by taking the componentId and modifying it during runtime, when extend or withComponent is called.

cc @mxstbr: Should I open a detailed issue for this on the styled-components repo for someone to pick up?

@mxstbr
Copy link
Member

mxstbr commented Jul 25, 2017

Sure

@kitten
Copy link
Member

kitten commented Jul 25, 2017

@mxstbr styled-components/styled-components#1031

@neoziro If you'd still like to pick it up, here it is :)

@gregberge
Copy link

@philpl thanks I will take a look as soon as possible!

@gregberge
Copy link

This PR should fix it styled-components/styled-components#1044

@pdeszynski
Copy link

@neoziro @philpl Is it already in the latest release? I have similar issue when using babel plugin 1.2.0 and styled components 2.1.2 also with ssr: true. I'm getting this issue only when exporting styled component as default and using className prop, for example, this doesn't work:

import React from 'react';
import PropTypes from 'prop-types';
import styled from 'styled-components';

class Contact extends React.Component {
  static propTypes = {
    className: PropTypes.string.isRequired,
  };

  render() {
    const { className } = this.props;
    return (
      <div className={className}>
        <h1>
          Title
        </h1>
        <p>content</p>
      </div>
    );
  }
}

export default styled(Contact)`
  color: black;
`;

and gives diff in class names:

 (client) <div class="Contact___default-s10ewv4h-...
 (server)<div class="Contact-s10ewv4h-0 etIOKa"... 

But just assigning it to a variable will work correctly:

const StyledContact = styled(Contact)`
   color: black;
`;

export default StyledContact;

@gregberge
Copy link

@piteer1 I think it's a bug. But I recommend you to assign a variable first to have a correct displayName in React debugging tools.

@rtymchyk
Copy link
Author

rtymchyk commented Sep 8, 2017

@piteer1 ☝️ I started to name exports to get around the error.

@mxstbr
Copy link
Member

mxstbr commented Sep 8, 2017

This sounds like a bug @piteer1, would you mind submitting that code as a failing test as a PR so we can reproduce & fix it? Thanks!

@pdeszynski
Copy link

@mxstbr What's the expected displayName for a component in that case? With __default or without it?

@mxstbr
Copy link
Member

mxstbr commented Sep 8, 2017

Ohh maybe thats's why it breaks? You're possibly running a ES6 module transform first, then the Babel plugin, on the client, and on the server you're running the Babel plugin first, then the module transform. (or the other way around) That's why in one it includes __default and in the other one it doesn't maybe?

Anyways, I think the right way to go™️ would be to not include it and only use the filename? Check the existing tests first though, I'm not 100% sure of the up-/downsides of that.

@pdeszynski
Copy link

For now I'm struggling to make a failing test as everything looks to be working fine even with default export. I think this is related to Webpack and the differences how it transpiles code for a web vs node. In client I'm getting:

// client.js
// ....
var Component = function (_React$Component) {
  _inherits(Component, _React$Component);

  function Component() {
    _classCallCheck(this, Component);

    return _possibleConstructorReturn(this, (Component.__proto__ || Object.getPrototypeOf(Component)).apply(this, arguments));
  }

  _createClass(Component, [{
    key: 'render',
    value: function render() {
      var className = this.props.className;

      return __WEBPACK_IMPORTED_MODULE_0_react___default.a.createElement(
        'div',
        { className: className },
        '...'
      );
    }
  }]);

  return Component;
}(__WEBPACK_IMPORTED_MODULE_0_react___default.a.Component);

Component.propTypes = {
  className: __WEBPACK_IMPORTED_MODULE_1_prop_types___default.a.string.isRequired
};

var _default = Object(__WEBPACK_IMPORTED_MODULE_2_styled_components__["default"])(Component).withConfig({
  displayName: 'Component___default',
  componentId: 'zld3iy-0'
})(['color:black;']);

But the node target output looks a little bit differently:

// component.chunk.js
class Component extends __WEBPACK_IMPORTED_MODULE_0_react___default.a.Component {

  render() {
    const { className } = this.props;
    return __WEBPACK_IMPORTED_MODULE_0_react___default.a.createElement(
      'div',
      { className: className },
      '...'
    );
  }
}

Component.propTypes = {
  className: __WEBPACK_IMPORTED_MODULE_1_prop_types___default.a.string.isRequired
};
/* harmony default export */ __webpack_exports__["a"] = (__WEBPACK_IMPORTED_MODULE_2_styled_components___default()(Component).withConfig({
  displayName: 'Component',
  componentId: 'zld3iy-0'
})(['color:black;']));

It's probably because of assignment var _default = Object(... in Webpack's generated code which is interpreted as the name of a component. If there's an assignment to a var then in both web/node then the output code is more similar so it works perfectly in that case.

@mxstbr
Copy link
Member

mxstbr commented Sep 9, 2017

Can you maybe submit those two generated code snippets (or a small version of them that reproduces the problem) as tests? That way we can make sure they align when we choose the variable names!

@gregberge
Copy link

@piteer1 @mxstbr A quick workaround could be ignoring "_default" as display name.

@gregberge
Copy link

Not very very clean, but the most efficient I think.

@pdeszynski
Copy link

@mxstbr Sorry for late answer, but I wanted to know what exactly causes this behavior. It is React Hot Loader plugin that does this transformation with var _default. I was able to create a failing test at last: #86

Because it only happens when this plugin is attached, I'm not sure if it should be considered as a bug in this plugin.

@gregberge
Copy link

React Hot Loader is widely used, so I think we should ignore _default, it costs nothing.

@kitten
Copy link
Member

kitten commented Sep 10, 2017

@mxstbr @piteer1 @neoziro I've taken a look at the failing test and the problems seem to be:

  • No name (variable name) is given to the StyledComponent. This is generally bad practice as we're enforcing that every presentational component has a name.
  • The order of the plugins might be relevant (See below for an explanation)

So to the order of the plugins, what is happening is:

  1. We detect the StyledComponent and detect the filename. At this point it doesn't have a variable name, so we leave it blank. Only the filename is used
  2. The hot loader plugin creates a variable called _default for the component

Depending on the order we either have a variable name (_default) or not. So the solution is to keep the order of the babel plugin consistent.

@mxstbr We can add the variable name to be _default for StyledComponents that are being export default-ed. The problem is that other plugins might choose to change this pattern completely. So if another plugin chooses the variable name defaultExport or something similar, this won't fix anything and we'll be back to square one.

I think we can add _default for now, and output a warning that says something along the lines of:

The ssr option is turned on, but you're default-exporting a StyledComponent inside [FILENAME]. This can cause its name to be unstable when other plugins that are wrapping it are used. Please apply the best practice of creating a variable for your component.

Sounds good?

@kitten kitten self-assigned this Sep 10, 2017
@pdeszynski
Copy link

@philpl yeah, the warning would be great and for me it would be the best solution for this problem.

@mxstbr
Copy link
Member

mxstbr commented Sep 11, 2017

I think the warning makes it sound as if you shouldn't export default any styled components, which isn't really the case.

We should warn something like

[babel-plugin-styled-components] You've turned on the ssr option, but another Babel plugin has changed the exports. (e.g. react-hot-loader does this) Please make sure you're running the Babel plugins in the same order on the client and the server, or move the styled-components plugin as the first one in the array. See [this issue] for more information!

Also, thanks @piteer1 for finally figuring out the issue, your debugging is much much appreciated!

@pdeszynski
Copy link

@mxstbr about the plugins order, I didn't see any difference whether I had in .babelrc react-hot-loader/babel as first or last one in plugins array. The result was just the same. In the test I've posted hot-loader was after your plugin:

{
  "plugins": [
    ["../../../src", {
      "ssr": true
    }],
    "react-hot-loader/babel"
  ],
}

@kitten
Copy link
Member

kitten commented Sep 11, 2017

@mxstbr Yea, I think your message is also a bit ambiguous as it doesn't hint at the correct solution — assigning the component to a variable.

@piteer1 Regarding the ordering issue, it's supposed to run before. But I feared as much. We might not be able to change the ordering completely, since it could just be caused by the AST structure. I'm not to familiar with the order of execution of Babel plugins.

Let's iterate on the message. It'll need to state:

  • The user has turned on ssr (or displayName actually)
  • A component is being exported as a default (i.e. export default)
  • This component is not assigned to a variable and exported anonymously

@rtymchyk
Copy link
Author

rtymchyk commented Sep 11, 2017

@philpl Yes, enforcing order of when babel plugins execute is not really possible. http://thejameskyle.com/babel-plugin-ordering.html

@gregberge
Copy link

Warning sounds good.

@mxstbr
Copy link
Member

mxstbr commented Sep 11, 2017

Okay, two things:

  1. We need to mention that we can't fix this and it's an issue with how certain Babel plugins (like react-hot-loader) change exports around.
  2. Let's add a long message with code examples to the website under FAQ and link to it from the warning. ("Why am I getting checksum mismatches when server-side rendering?" or smth) This could also include more common cases like using window.x (e.g. window.innerWidth) in your styling etc. (maybe we should write an eslint plugin? lol)

Text for the warning:

[babel-plugin-styled-components] You've turned on the ssr option, but another Babel plugin has changed the exports. (e.g. react-hot-loader does this) This will break client-side rehydration. Please assign your component to a variable and export default the variable, see [link-to-website]

Code examples:

// Before
export default styled.button`
  color: blue;
`;

// After
const Button = styled.button`
  color: blue;
`
export default Button;

@kitten
Copy link
Member

kitten commented Sep 11, 2017

@mxstbr

"Why am I getting checksum mismatches when server-side rendering?"

I didn't even think of this. That's PERFECT! ❤️

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

No branches or pull requests

6 participants