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

Custom CacheProvider does not pick up styles from bundled components #1386

Closed
ChrisMccollister-minted opened this issue Jun 4, 2019 · 13 comments
Labels

Comments

@ChrisMccollister-minted
Copy link

Current behavior:

A custom CacheProvider does not pick up styles from components imported from bundled JS. Styles for these components are prefixed with the default css key and are injected into the document head, not the location specified in the cache.

To reproduce:

Repo with example: https://github.com/ChrisMccollister-minted/emotion-cache-provider-test

Expected behavior:

For the code shown below, I would expect the styles used by LibraryComponent to be prefixed with bbb and injected into the div holding the t2Ref.

import React, {Component} from 'react';
import ReactDOM from 'react-dom';
import {css, CacheProvider} from '@emotion/core';
import createCache from '@emotion/cache';
import LibraryComponent from 'emotion-cache-test-lib';

const localStyle = css`
  color: red;
`;

const LocalComponent = ({children}) => (
  <div css={localStyle}>{children}</div>
);

class Main extends Component {
  constructor(props) {
    super(props);
    this.t1Ref = React.createRef();
    this.t2Ref = React.createRef();
    this.state = {
      isReady: false,
    };
  }

  componentDidMount() {
    this.t1Cache = createCache({
      key: 'aaa',
      container: this.t1Ref.current,
    });
    this.t2Cache = createCache({
      key: 'bbb',
      container: this.t2Ref.current,
    });
    this.setState({isReady: true});
  }

  render() {
    const {isReady} = this.state;
    return (
      <div>
        <div ref={this.t1Ref}>
          {isReady &&
            <CacheProvider value={this.t1Cache}>
              <LocalComponent>
                LocalComponent
              </LocalComponent>
            </CacheProvider>
          }
        </div>
        <div ref={this.t2Ref}>
          {isReady &&
            <CacheProvider value={this.t2Cache}>
              <LibraryComponent>
                LibraryComponent
              </LibraryComponent>
            </CacheProvider>
          }
        </div>
      </div>
    );
  }
}

ReactDOM.render(
  <Main />,
  document.getElementById('root')
);

Looking at the resulting document, I see that the styles for LocalComponent are correctly prefixed with aaa and injected into the div holding the t1Ref. The styles used by LibraryComponent however, are prefixed with the default css prefix and injected into the document head, as if no custom CacheProvider was being used.

Screen Shot 2019-06-04 at 3 43 35 PM

If this is the intended behavior, then I would expect the docs to mention that CacheProvider does not work with bundled components.

Environment information:

  • react version: 16.8.6
  • @emotion/core version: 10.0.10
  • @emotion/cache version: 10.0.9
@Andarist
Copy link
Member

Andarist commented Jun 5, 2019

Can you reproduce this when installing LibraryComponent? I mean - when not using npm link?

I think the problem is that each project - a library and your app - has its own @emotion/core installed, so they actually have 2 different contexts (referentially speaking). This might happen when using npm link.

@ChrisMccollister-minted
Copy link
Author

ChrisMccollister-minted commented Jun 5, 2019

Yes, I can reproduce this when I'm installing the package instead of using link.

Edit: After further testing, I can get this to work as expected in my test repo if I'm not using yarn link. I'm still seeing issues with other bundles but I'm guessing that's due to a different configuration issue causing the same duplicate context situation.

@FezVrasta
Copy link
Member

FezVrasta commented Jun 6, 2019

@Andarist could we put the context into some global space to avoid this issue? I know is not going to be nice, but maybe just for dev builds we could put it into window.__EMOTION10_CONTEXT__

@Andarist
Copy link
Member

Andarist commented Jun 6, 2019

Edit: After further testing, I can get this to work as expected in my test repo if I'm not using yarn link. I'm still seeing issues with other bundles but I'm guessing that's due to a different configuration issue causing the same duplicate context situation.

That has to be it. You can try to find the culprits and resolve your dependency ranges in such a way that u would have a single version of emotion@10 in your node_modules. If you are using webpack I would advise aliasing it (and other shared react libs and react itself) to resolved paths. I'm doing this it with something like this:

const path = require('path')
const fs = require('fs')

function findPkgRoot(directory) {
  const configPath = path.join(directory, 'package.json')

  if (fs.existsSync(configPath)) {
    return directory
  }

  return findPkgRoot(path.join(directory, '..'))
}

const dependencies = Object.keys(require(path.join(findPkgRoot(__dirname), 'package.json')).dependencies)

const forcedNodeModules = dependencies..reduce((acc, dep) => {
  acc[dep] = findPkgRoot(require.resolve(dep))
  return acc
}, {})

module.exports = {
  // webpack config
  // ...
  resolve: {
    // ...
    alias: forcedNodeModules
  }
}

@Andarist could we put the context into some global space to avoid this issue? I know is not going to be nice, but maybe just for dev builds we could put it into window.EMOTION10_CONTEXT

IMHO that wouldn't solve the issue completely and could introduce some bizarre bugs into somebody's setup. We can't assume how people differentiate their dev & prod builds, so any attempt to fix the dev one might just hide the issue and cause breakage accidentally in prod.

We could use this to produce a dev only warning though, that we have detected multiple instances of our package being loaded.

Keep in mind that the problem here is not unique to emotion itself, anything that relies on same references can break - i.e. instanceof checks are somewhat unreliable for that reason (when checking instanceof of something external). Also react itself "suffers" from it - bad things might happen when including multiple versions of react, especially when using hooks as they rely on shared scope inside of the react modules.

@FezVrasta
Copy link
Member

A warning would be great!

@Andarist
Copy link
Member

There is nothing that we can do to fix this - it's just how things work in general (the issue is not exclusive to emotion). We could introduce some kind of a warning - which is tracked here #1470 , but we need to think about all pros and cons of this before implementing this.

@zeckdude
Copy link

zeckdude commented Mar 17, 2020

This works for JSS btw, so I'm not convinced this issue is how things work in general. I couldn't tell you what the solution is but they made it work. Basically I have some code in the shadow dom in which I am using components made in Material UI (which uses JSS behind the scenes) that are from an external package and I am able to specify the location where those styles should be inserted. Not only does it insert all styles that are created within the shadow dom but also the ones that are imported via the package.

Unfortunately the solution that allows me to specify where to add the emotion styles that are created within the shadow dom does not also include the emotion styles that are imported via the package.

import root from 'react-shadow';
import { jssPreset, StylesProvider as MuiStylesProvider, MuiThemeProvider } from '@material-ui/core/styles';
import { ThemeProvider as EmotionThemeProvider } from 'emotion-theming';
import { create } from 'jss';
import React, { useState } from 'react';
import { CacheProvider as EmotionCacheProvider } from '@emotion/core';
import createCache from '@emotion/cache';
import { Theme, Components } from 'design-library';

// Define custom location to insert JSS styles (instead of document head)
// From: https://stackoverflow.com/a/57221293/83916

// Define custom location to insert Emotion styles (instead of document head)
// From: https://emotion.sh/docs/cache-provider

const ShadowDomContainer = ({ children }) => {
  const [jss, setJss] = useState(null);
  const [emotionCache, setEmotionCache] = useState(null);

  function createJssStyles(headRef) {
    if (headRef && !jss) {
      const createdJssWithRef = create({ ...jssPreset(), insertionPoint: headRef });
      setJss(createdJssWithRef);
    }
  }

  function setEmotionStyles(emotionRef) {
    if (emotionRef && !emotionCache) {
      const createdEmotionWithRef = createCache({
        container: emotionRef,
      });
      setEmotionCache(createdEmotionWithRef);
    }
  }

  function setShadowHeadRef(headRef) {
    createJssStyles(headRef);
    setEmotionStyles(headRef);
  }

  return (
    <root.div>
      <head ref={setShadowHeadRef} />
      {jss && emotionCache && (
        <MuiThemeProvider theme={Theme}>
          <EmotionThemeProvider theme={Theme}>
            <EmotionCacheProvider value={emotionCache}>
              <MuiStylesProvider jss={jss}>
                <Components.ScopedCssBaseline>{children}</Components.ScopedCssBaseline>
              </MuiStylesProvider>
            </EmotionCacheProvider>
          </EmotionThemeProvider>
        </MuiThemeProvider>
      )}
    </root.div>
  );
};

export default ShadowDomContainer;

@Andarist
Copy link
Member

The "double context" problem is literally the only way this can break. I would encourage you to check your node_modules for multiple @emotion/core packages.

@zeckdude
Copy link

@Andarist If that is the case, how would I solve the multiple packages issue?

@glomotion
Copy link

I wonder, could someone help me, i'm finding the same problem.

I have a design system lib which uses @emotion/css.

Im consuming said design system from this repo:
https://github.com/glomotion/test-emotion-cypress-7-x

Im trying to have the @emotion/css styles generated inside the DS - injected into the document.body, instead of the document.head. Is this possible?

@Andarist
Copy link
Member

@glomotion your problem is very different from the issue described here. @emotion/css is eager so as soon as you call css stuff gets injected into the DOM - so it's not as flexible as @emotion/react with <CacheProvider/>.

You can use a hack like this: https://codesandbox.io/s/twilight-glade-0tr4l?file=/src/App.js . But remember that this is a hack and this won't work correctly with things like SSR.

@glomotion
Copy link

Oh that is perfect!

In this case a Hack is fine, its really only needed to work around an issue with cypress component testing.

Thank you @Andarist!

@tho-graf
Copy link

Hi @Andarist. we stumbled on a very similar issue. Our problem was that we had different modules. Our component lib had commonjs and our frontend esm.

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