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

extractCritical does not seem to work when npm linking #349

Closed
tizmagik opened this issue Sep 26, 2017 · 8 comments · Fixed by #464
Closed

extractCritical does not seem to work when npm linking #349

tizmagik opened this issue Sep 26, 2017 · 8 comments · Fixed by #464

Comments

@tizmagik
Copy link

tizmagik commented Sep 26, 2017

  • emotion version: 7.3.2
  • react version: 16 (but also observed in latest 15.x)

Relevant code.

What you did:

import React from 'react';
import { renderToString } from 'react-dom/server';
import { extractCritical } from 'emotion/server';

// ... some more setup and usual stuff here ...

const markup = renderToString(
    <StaticRouter context={context} location={req.url}>
      <App />
    </StaticRouter>
);

const { css, ids } = extractCritical(markup);

console.log("css", css);
console.log("ids", ids);

// css and ids are blank/empty

What happened:

npm linking a shared component repo (styled with emotion) into a real app causes extractCritical to not work (e.g. ids and css are blank/empty)

Reproduction:

https://github.com/tizmagik/emotion-link-repro

Problem description:

npm link appears to break extractCritical

Suggested solution:

I'm not sure if it's actually a problem with extractCritical or just with my setup (likely the latter), but any pointers welcome!

@tkh44
Copy link
Member

tkh44 commented Sep 26, 2017

You need to be using the same instance of emotion. If you are linking then emotion-server is trying to link from its deps. Try linking emotion too(?)

@tizmagik
Copy link
Author

Ah, that might be it. Thanks for the quick response!

What's the recommended pattern here, in that case? We were hoping to keep emotion as a peerDependency, which works when building for production, but seems to break when developing locally. How would we also link emotion in that case?

@tizmagik
Copy link
Author

Also, any ideas why it needs to be the same instance of emotion? I thought it was mostly just a regex on the rendered HTML 🤔

@tkh44
Copy link
Member

tkh44 commented Sep 26, 2017

Emotion has a mock stylesheet called sheet that is used to collect the styles generated while your app is rendering. emotion-server reads the rules from the sheet when writing the critical css string.

@tkh44
Copy link
Member

tkh44 commented Sep 26, 2017

I looked through your reproduction repo and couldn't figure out a way to make it work off the top of my head. Maybe @mitchellhamilton has some ideas.

@tizmagik
Copy link
Author

Thanks for taking a look @tkh44 , appreciated. Do you know if this changes at all in Emotion 8?

@emmatown
Copy link
Member

emmatown commented Oct 8, 2017

@tizmagik Sadly, this won't change with emotion 8, the problem here is that the components package will always use its own version of emotion when linking since it'll be in its node_modules.

I have one idea of a way to get around this, I wouldn't exactly say it's a very good solution but anyway, have a folder with a package.json with emotion and react-emotion as dependencies and have a folder inside that with the actual package and have emotion and react-emotion as peerDependencies, using npm link on the package should work since it'll use the instance of emotion from where it's linked. This could also be solved with a monorepo using lerna or something and having emotion and react-emotion as dependencies in the root package.json.

@ericmasiello
Copy link

+1 for this issue.

I'm currently rebuilding our component library and evaluating different CSS in JS libraries. Before I publish any changes to npm, I wanna test this out on a few of our different projects that use the component library - one of which being a server side rendering app.

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

Successfully merging a pull request may close this issue.

4 participants