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

Styles not generated for Style Objects in a Node VM context #3066

Open
keeganstreet opened this issue Mar 17, 2020 · 14 comments · May be fixed by #3068
Open

Styles not generated for Style Objects in a Node VM context #3066

keeganstreet opened this issue Mar 17, 2020 · 14 comments · May be fixed by #3068
Labels

Comments

@keeganstreet
Copy link

@keeganstreet keeganstreet commented Mar 17, 2020

Styles are not generated for components styled with Style Objects when run in a Node VM context. Components styled with tagged template literals work perfectly well in the same Node VM context.

This is because the isPlainObject function tests whether the constructor of an object is Object. If an object is created in a VM context, it's constructor Object is not equal to the Object prototype object in the main context.

It would be great if this function could be updated so that it doesn't rely on being executed in the same JavaScript context that the component was created in.

Test case

npm i react react-dom styled-components

Run the following in Node:

const React = require("react");
const { renderToString } = require("react-dom/server");
const { default: styled, ServerStyleSheet } = require("styled-components");
const vm = require("vm");

const context = vm.createContext({ styled, React, App: null });

vm.runInContext(`
  const Green = styled.div\`
    color: green;
  \`;

  const Red = styled.div(() => ({
    color: "red"
  }));

  App = React.createElement("div", null, [
    React.createElement(Green, { key: 0 }, null),
    React.createElement(Red, { key: 1 }, null)
  ]);
`, context);

const sheet = new ServerStyleSheet();
renderToString(sheet.collectStyles(context.App));
const styleTags = sheet.getStyleTags();
console.log(styleTags);

Expected output

<style data-styled="true" data-styled-version="5.0.1">.lbxPDC{color:green;}
data-styled.g1[id="sc-AxjAm"]{content:"lbxPDC,"}
.dahFIE{color:red;}
data-styled.g2[id="sc-AxirZ"]{content:"dahFIE,"}
</style>

Actual output

[object Object]; is being rendered instead of color:red;.

<style data-styled="true" data-styled-version="5.0.1">.lbxPDC{color:green;}
data-styled.g1[id="sc-AxjAm"]{content:"lbxPDC,"}
.dahFIE{[object Object];}
data-styled.g2[id="sc-AxirZ"]{content:"dahFIE,"}
</style>
@kitten

This comment has been minimized.

Copy link
Member

@kitten kitten commented Mar 17, 2020

To be honest, I'm not quite sure why we have isPlainObject there, cc @probablyup. Maybe it was added when the order between the isStyledComponent check was reversed? At least it seems overkill to me either way

@kitten kitten added the bug 🐛 label Mar 17, 2020
@probablyup

This comment has been minimized.

Copy link
Contributor

@probablyup probablyup commented Mar 17, 2020

I'm fine with switching it to Object.prototype.toString.call(x) === '[object Object]' which will reliably give the correct answer across environments

@keeganstreet

This comment has been minimized.

Copy link
Author

@keeganstreet keeganstreet commented Mar 18, 2020

Thanks @kitten & @probablyup. Would you like a PR to make that change?

@probablyup

This comment has been minimized.

Copy link
Contributor

@probablyup probablyup commented Mar 18, 2020

Sure, thank you.

@keeganstreet

This comment has been minimized.

Copy link
Author

@keeganstreet keeganstreet commented Mar 19, 2020

Object.prototype.toString.call(x) will return "[object Object]" for instances too, i.e.:

class SomeClass {}
Object.prototype.toString.call(new SomeClass()); // "[object Object]"

🤔 Do you know what value x might be if it is not a plain object? Maybe we should check that x is not of that type rather than checking that it is a plain object.

@probablyup

This comment has been minimized.

Copy link
Contributor

@probablyup probablyup commented Mar 19, 2020

@kitten

This comment has been minimized.

Copy link
Member

@kitten kitten commented Mar 19, 2020

@probablyup I think we're fighting an uphill battle here. We already have a pretty comprehensive failure case: https://github.com/styled-components/styled-components/blob/master/packages/styled-components/src/utils/addUnitIfNeeded.js#L8

We stringify the object in a failure case, which is intended, so we are supporting toString making this a little more complicated. But we can modify flatten.js with a couple of things in mind first:

  • falsey values are not an issue and checked for (in both objToCssArray and flatten)
  • the function check can come first (that's a modification for objToCssArray)

Then we can assume that we either stringify or call objToCssArray to convert the object to CSS declarations. We could call typeOf from react-is to check whether it's a React element (or other type) which rules out the most common mistakes.

Then we can assume that if someone passes new SomeClass then that was intentional. We only have to figure out whether they wanted to stringify new SomeClass or have it converted to CSS. For that I'd propose we'd just check:

typeof x === 'object' && x.toString === Object.prototype.toString

If this holds up we call objToCssArray, otherwise we drop into addUnitIfNeeded (the string case) since clearly the user has a custom toString function. This also seems to hold up for the vm module btw.

We don't have to handle arrays any differently. They aren't expected in objToCssArray and are currently (maybe erroneously) stringified. They are handled upfront in the flatten function however.

We also don't have to handle symbols. They aren't handled right now and there's a low to no chance that they'll be passed as an interpolation.

@probablyup

This comment has been minimized.

Copy link
Contributor

@probablyup probablyup commented Mar 19, 2020

@kitten

This comment has been minimized.

Copy link
Member

@kitten kitten commented Mar 19, 2020

Quick note on what this would look like:

import { typeOf } from 'react-is';

const isPlainObject = x =>
  x !== null &&
  typeof x === 'object' &&
  !typeOf(x) &&
  x.toString === Object.prototype.toString

At which point we should rename it, maybe to isSerializableObject or isCSSObject.

The toString approach on its own isn't sufficient:

Object.prototype.toString.call(React.createElement('div'))
// '[object Object]'

And x.toString === Object.prototype.toString does the trick in vm as well without calling toString on random objects 😅

@probablyup

This comment has been minimized.

Copy link
Contributor

@probablyup probablyup commented Mar 19, 2020

@kitten

This comment has been minimized.

Copy link
Member

@kitten kitten commented Mar 19, 2020

Well, we do have to cover React-ish stuff, so that's fine. And it'd be great to consistently support custom toString functions.

Also, this really doesn't add a lot of weight, we can even drop x !== null and we arrive at something that is barely longer compared to other size savings we could have

@keeganstreet

This comment has been minimized.

Copy link
Author

@keeganstreet keeganstreet commented Mar 19, 2020

@kitten x.toString === Object.prototype.toString will have the same problem as x.constructor === Object in terms of relying on x being created in the same context as this conditional.

@kitten

This comment has been minimized.

Copy link
Member

@kitten kitten commented Mar 19, 2020

Good point, I double checked this and you're absolutely right 👍 I must've misremembered that native functions share the same instances in different contexts.

Edit: that being said, let's please add an explicit element check at least to avoid infinite recursion 😅

@keeganstreet

This comment has been minimized.

Copy link
Author

@keeganstreet keeganstreet commented Mar 19, 2020

So maybe something like this?

import { typeOf } from 'react-is';

const isSerializableObject = (x: any): boolean =>
  x !== null &&
  typeof x === 'object' &&
  x.toString() === '[object Object]' &&
  !typeOf(x);

!typeOf(x) to make sure it's not a React element.

I used x.toString() === '[object Object]' instead of Object.prototype.toString.call(x) === '[object Object]' to avoid failing the unit test flatten › toStrings class instances. If an object defines its own toString method I think it makes sense to call it rather than toString from the Object.prototype object.

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

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.