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

Emotion types are polluting the global scope #1800

Closed
fatfisz opened this issue Mar 12, 2020 · 14 comments
Closed

Emotion types are polluting the global scope #1800

fatfisz opened this issue Mar 12, 2020 · 14 comments
Labels

Comments

@fatfisz
Copy link

fatfisz commented Mar 12, 2020

Current behavior:

The @emotion/core/types/index.d.ts file from the package includes the following declarations:

declare module 'react' {
  interface DOMAttributes<T> {
    css?: InterpolationWithTheme<any>
  }
}

declare global {
  namespace JSX {
    /**
     * Do we need to modify `LibraryManagedAttributes` too,
     * to make `className` props optional when `css` props is specified?
     */

    interface IntrinsicAttributes {
      css?: InterpolationWithTheme<any>
    }
  }
}

This causes the global scope to become polluted with @emotion types, even though the user might not be asking for it - eg. it came as a surprise to me, because @emotion is a dependency of storybook, and I didn't know about it until I tried using the css prop for styled-components and there was no error.

Further, after including styled-components type declaration for the css prop, some strange errors emerged because of the way TypeScript handles declaration merging:

  Types of property 'css' are incompatible.
    Type 'string | CSSObject | FlattenInterpolation<ThemeProps<any>> | undefined' is not
assignable to type 'InterpolationWithTheme<any>'.
      Type 'FlattenInterpolation<ThemeProps<any>>' is not assignable to type 'InterpolationWithTheme<any>'.
        Type 'FlattenInterpolation<ThemeProps<any>>' is not assignable to type 'ObjectInterpolation<undefined>'.

Expected behavior:

I'd expect a mechanism similar to styled-components/cssprop: the user has to manually create a file which imports global declarations instead of pullution by default.

Environment information:

@emotion/[email protected]

@osdiab
Copy link
Contributor

osdiab commented Mar 13, 2020

As a workaround I think you can use a whitelist of global types by providing the types config in TSConfig, which I think would allow you to not automatically include random globals included by other libraries

@fatfisz
Copy link
Author

fatfisz commented Mar 13, 2020

Unfortunately this doesn't work:

  1. We're importing from @storybook/addon-knobs
  2. That file references @storybook/addon-knobs/dist/components/types/index.d.ts
  3. Which in turn has this line inside: /// <reference types="@emotion/core" />

... so TypeScript imports the file anyway.

Now that I think of it, even if you switch to an opt-in behavior, Storybook will still need to avoid polluting the global scope by explicitly providing a type for the css prop as opposed to hypothetically importing @emotion/core/cssprop. But that's the next step.

So for now I guess I'll stick to rm @emotion/.../index.d.ts in postinstall in package.json 😂

@Andarist
Copy link
Member

Andarist commented Mar 13, 2020

Is there a way to stop TS from importing a file automatically? Actually, when I've re-read your answer @fatfisz I've noticed you have added some nice extra info I was missing before. That @storybook includes the reference to @emotion/core.

I'm going to think about how this can be solved, but if you have any ideas of your own I would highly appreciate any help with this.

This is also a duplicate of #1257

@Andarist
Copy link
Member

One more thing, would you be able to prepare a minimal repro case for this? It would allow me to jump into this issue much sooner.

@fatfisz
Copy link
Author

fatfisz commented Mar 14, 2020

Sorry, I didn't manage to find the previous issue. My suggested solution is the same as styled-components:

  1. Export a type called CSSProp or similar that will be equal to InterpolationWithTheme<any>
  2. Provide a separate type file like styled-components/cssprop that injects the css?: CSSProp property into the global scope. In the case of styled-components it's not in the library itself, but rather in @types/styled-components, but it shouldn't matter

The benefits I see here are:

  • Opt-in instead of opt-out
  • If there's a requirement to limit the usage of css to only a few components (eg. because of a migration from another tool or in the case of Storybook to prevent the global leak), one can always refer to CSSProp directly

Here's my smallest reproduction repo: https://github.com/fatfisz/emotion-global-type-injection-repro.

@Andarist
Copy link
Member

Provide a separate type file like styled-components/cssprop that injects the css?: CSSProp property into the global scope. In the case of styled-components it's not in the library itself, but rather in @types/styled-components, but it shouldn't matter

The problem I see with that is that you would still probably bump into this issue because StoryBooks would still refer to this file and would drag this global pollution into your build, wouldn't it?

@fatfisz
Copy link
Author

fatfisz commented Mar 14, 2020

Yes, as I mentioned previously:

Now that I think of it, even if you switch to an opt-in behavior, Storybook will still need to avoid polluting the global scope by explicitly providing a type for the css prop as opposed to hypothetically importing @emotion/core/cssprop. But that's the next step.

But without Emotion fixing the problem first, Storybook can't do anything. I can only promise that I'll take it up with them afterwards 🙂

@Andarist
Copy link
Member

But even if we are going to fix it in a way you have proposed I'm still unsure how they could utilize css prop without polluting the global types. What would be the recommendation for them if we make css prop types opt-in?

@fatfisz
Copy link
Author

fatfisz commented Mar 14, 2020

Each of the exported components that is actually using Emotion's css should explicitly have it in their interface; I don't know much about the structure of that project, but on a quick glance KnobControlProps seems like a good place, because all knob components extend it:

import { CSSProp } from '@emotion/...'; // Just importing the type shouldn't pollute the global scope

export interface KnobControlProps<T> {
    css?: CSSProp; // Just define the css prop explicitly here
    knob: KnobControlConfig<T>;
    onChange: (value: T) => T;
}

@gabrieledarrigo
Copy link

gabrieledarrigo commented Apr 1, 2020

Any news on this?
It's quite frustrating since there's no clean way to solve this problem while using styled-components.

@robphoenix
Copy link

I've just run into this building a component library on top of theme-ui. I have a <Box> component with a css prop that passes styles to theme-ui's sx prop. For this to work I have to comment out a line in the @emotion/core/types/index.d.ts file.

declare global {
  namespace JSX {
    /**
     * Do we need to modify `LibraryManagedAttributes` too,
     * to make `className` props optional when `css` props is specified?
     */

    interface IntrinsicAttributes {
      // css?: InterpolationWithTheme<any>
    }
  }
}

@Nvveen
Copy link

Nvveen commented Jun 20, 2020

I have this problem with styled-components too, where I have to explicitly annotate props in a template string, even though styled-components itself can properly infer the type. If I manually remove the *.d.ts files in @emotion, the problem goes away. The strange thing is, though, that VSCode does properly infer the props type when on mouseover of the parameter. For now, the only solution I have is a postinstall rm -rf.

@Andarist
Copy link
Member

I believe I have found an appropriate fix for this on our end - if anyone wants to take a look then here it is: #1941 . Keep in mind that it targets the upcoming v11 that we hope to ship soon.

@Andarist
Copy link
Member

A fix for this has been just merged into v11 line: #1941

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