Closed rtymchyk closed 5 years ago
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
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.
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.
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
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?
@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?
Sure
@mxstbr https://github.com/styled-components/styled-components/issues/1031
@neoziro If you'd still like to pick it up, here it is :)
@philpl thanks I will take a look as soon as possible!
This PR should fix it https://github.com/styled-components/styled-components/pull/1044
@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;
@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.
@piteer1 ☝️ I started to name exports to get around the error.
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!
@mxstbr What's the expected displayName
for a component in that case? With __default
or without it?
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.
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.
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!
@piteer1 @mxstbr A quick workaround could be ignoring "_default" as display name.
Not very very clean, but the most efficient I think.
@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: https://github.com/styled-components/babel-plugin-styled-components/pull/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.
React Hot Loader is widely used, so I think we should ignore _default, it costs nothing.
@mxstbr @piteer1 @neoziro I've taken a look at the failing test and the problems seem to be:
StyledComponent
. This is generally bad practice as we're enforcing that every presentational component has a name.So to the order of the plugins, what is happening is:
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_default
for the componentDepending 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 StyledComponent
s 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?
@philpl yeah, the warning would be great and for me it would be the best solution for this problem.
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!
@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"
],
}
@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:
ssr
(or displayName
actually)export default
)@philpl Yes, enforcing order of when babel plugins execute is not really possible. http://thejameskyle.com/babel-plugin-ordering.html
Warning sounds good.
Okay, two things:
react-hot-loader
) change exports around.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;
@mxstbr
"Why am I getting checksum mismatches when server-side rendering?"
I didn't even think of this. That's PERFECT! :heart:
I am using the babel plugin
1.1.5
with styled components2.1.0
. I have placed assr: true
in both the client and the server babel configs.However, I always see this mismatch in checksum:
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?