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

Avoid referencing internal Emotion packages in built types #16905

Merged
merged 2 commits into from
Dec 10, 2021

Conversation

Methuselah96
Copy link
Contributor

@Methuselah96 Methuselah96 commented Dec 4, 2021

Issue: N/A

What I did

Errors I'm trying to fix when using Yarn PnP:

../../.yarn/__virtual__/@storybook-theming-virtual-8ade4919eb/0/cache/@storybook-theming-npm-6.4.7-455113f13a-4e2300146b.zip/node_modules/@storybook/theming/dist/ts3.9/animation.d.ts:5:23 - error TS2307: Cannot find module '@emotion/serialize' or its corresponding type declarations.

5     rotate360: import("@emotion/serialize").Keyframes;
                        ~~~~~~~~~~~~~~~~~~~~

../../.yarn/__virtual__/@storybook-theming-virtual-8ade4919eb/0/cache/@storybook-theming-npm-6.4.7-455113f13a-4e2300146b.zip/node_modules/@storybook/theming/dist/ts3.9/animation.d.ts:6:18 - error TS2307: Cannot find module '@emotion/serialize' or its corresponding type declarations.

6     glow: import("@emotion/serialize").Keyframes;
                   ~~~~~~~~~~~~~~~~~~~~

../../.yarn/__virtual__/@storybook-theming-virtual-8ade4919eb/0/cache/@storybook-theming-npm-6.4.7-455113f13a-4e2300146b.zip/node_modules/@storybook/theming/dist/ts3.9/animation.d.ts:7:19 - error TS2307: Cannot find module '@emotion/serialize' or its corresponding type declarations.

7     float: import("@emotion/serialize").Keyframes;
                    ~~~~~~~~~~~~~~~~~~~~

../../.yarn/__virtual__/@storybook-theming-virtual-8ade4919eb/0/cache/@storybook-theming-npm-6.4.7-455113f13a-4e2300146b.zip/node_modules/@storybook/theming/dist/ts3.9/animation.d.ts:8:20 - error TS2307: Cannot find module '@emotion/serialize' or its corresponding type declarations.

8     jiggle: import("@emotion/serialize").Keyframes;
                     ~~~~~~~~~~~~~~~~~~~~

../../.yarn/__virtual__/@storybook-theming-virtual-8ade4919eb/0/cache/@storybook-theming-npm-6.4.7-455113f13a-4e2300146b.zip/node_modules/@storybook/theming/dist/ts3.9/animation.d.ts:9:24 - error TS2307: Cannot find module '@emotion/utils' or its corresponding type declarations.

9     inlineGlow: import("@emotion/utils").SerializedStyles;
                         ~~~~~~~~~~~~~~~~

../../.yarn/__virtual__/@storybook-theming-virtual-8ade4919eb/0/cache/@storybook-theming-npm-6.4.7-455113f13a-4e2300146b.zip/node_modules/@storybook/theming/dist/ts3.9/animation.d.ts:10:23 - error TS2307: Cannot find module '@emotion/utils' or its corresponding type declarations.

10     hoverable: import("@emotion/utils").SerializedStyles;
                         ~~~~~~~~~~~~~~~~


Found 6 errors.

This happens because lib/theming/src/animation.d.ts ends up referencing these packages like this:

export declare const easing: {
    rubber: string;
};
export declare const animation: {
    rotate360: import("@emotion/serialize").Keyframes;
    glow: import("@emotion/serialize").Keyframes;
    float: import("@emotion/serialize").Keyframes;
    jiggle: import("@emotion/serialize").Keyframes;
    inlineGlow: import("@emotion/utils").SerializedStyles;
    hoverable: import("@emotion/utils").SerializedStyles;
};

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Dec 4, 2021

Nx Cloud Report

CI ran the following commands for commit 4665e59. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@yannbf
Copy link
Member

yannbf commented Dec 6, 2021

Hey @Methuselah96 thank you so much for your contribution!

@Andarist could you please take a look at this? Thank you!

@Andarist
Copy link
Contributor

Andarist commented Dec 6, 2021

@yannbf this is a somewhat interesting case. I understand why the issue is there with PnP and I think this should be fixed one way or another. I find it problematic though that TS even allows this output to be emitted - this IMHO should be a compilation error because the imported symbol is somewhat implicit. From your perspective, as the author, @emotion/serialize doesn't even exist - it's an implementation detail and you shouldn't be forced to add this to your dependencies.

AFAIK somewhat similar issues will start to pop up as errors once TS ship the support for package.json#exports (this is already available in their nightly channel) and the solution for those other problems will be to... make dependencies for your exported symbols explicit.

And while the proposed fix here could potentially fix every potential problem at once, I think that the better solution is the one that I'm going to propose. YMMV though so choose whatever feels best for your product.

Instead of adding this to your dependencies, I would do this:

type Keyframes = ReturnType<typeof keyframes>

// annotate your stuff explicitly
const rotate360: Keyframes = keyframes`...`

A part of the problem might be that @emotion/core doesn't reexport Keyframes - I suspect that if it would then the emitted output would look different and the issue wouldn't be there. You are still using Emotion 10 though and I kinda would like to avoid releasing any unnecessary patches there. However, if you would test such a change out to confirm if that changes the TS emit output here and prepare a PR to Emotion with such a change (targeting v10 branch) then I would probably merge this in and release for you.

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Dec 7, 2021

@Andarist Using ReturnType<typeof keyframes> and ReturnType<typeof css> didn't seem to make a difference on how TypeScript outputted the declaration file. It still imported the underlying packages for both types. I've opened emotion-js/emotion#2576 for v10 and emotion-js/emotion#2577 for v11 so that Storybook can import Keyframes and SerializedStyles from @emotion/core and just declare the types for those variables.

@Methuselah96
Copy link
Contributor Author

I did some more investigation and it looks like starting in TypeScript 4.2 it picks up on SerializedStyles being re-exported and uses @emotion/core instead of @emotion/utils (Storybook is on TypeScript 3.9), but Keyframes still has the issue because it's not re-exported. Still need to look into why ReturnType doesn't work as expected (it works on TS Playground but not when building the types in Storybook).

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Dec 8, 2021

It turns out that ReturnType<typeof keyframes> only works if an intermediate interface is declared.

Without interface (TS playground):

import { css, keyframes } from '@emotion/react';

type Keyframes = ReturnType<typeof keyframes>;
type SerializedStyles = ReturnType<typeof css>;

const testKeyframes: Keyframes = keyframes``;
const testCSS: SerializedStyles = css``;

export const animations = {
    testKeyframes,
    testCSS,
};

Produces:

export declare const animations: {
    testKeyframes: import("@emotion/serialize/types").Keyframes;
    testCSS: import("@emotion/react").SerializedStyles;
};

With interface (TS playground):

import { css, keyframes } from '@emotion/react';

type Keyframes = ReturnType<typeof keyframes>;
type SerializedStyles = ReturnType<typeof css>;

const testKeyframes = keyframes``;
const testCSS = css``;

interface Animations {
    testKeyframes: Keyframes;
    testCSS: SerializedStyles
}

export const animations: Animations = {
    testKeyframes,
    testCSS,
};

Produces:

import { css, keyframes } from '@emotion/react';
declare type Keyframes = ReturnType<typeof keyframes>;
declare type SerializedStyles = ReturnType<typeof css>;
interface Animations {
    testKeyframes: Keyframes;
    testCSS: SerializedStyles;
}
export declare const animations: Animations;
export {};

@yannbf @Andarist How would you like to proceed? The options are (from least to most preferable in my opinion):

  1. Add dependencies.
  2. Introduce intermediate Animations interface that uses ReturnType<typeof animations>.
  3. Export Keyframes type from @emotion/core (v10) / @emotion/react (v11). This would require no changes to Storybook (besides updating the package) and seems preferable to me since it's a more general solution that could fix it for me people/situations than just this one. Obviously comes at the cost of releasing a patch to Emotion 10, but seems like this should be done for Emotion 11 regardless of which solution we go with for Emotion 10.

@Methuselah96 Methuselah96 changed the title Add missing dependencies on emotion packages for types Avoid referencing internal Emotion packages in built types Dec 8, 2021
@Andarist
Copy link
Contributor

Andarist commented Dec 8, 2021

I've just released @emotion/core@10.3.1 - you should be able to install it to avoid this issue altogether. Please let me know if this didn't do the trick for some reason

@Methuselah96
Copy link
Contributor Author

@Andarist That's very helpful, thanks! We still have to declare the type for each of the variables because TypeScript 3.9 doesn't figure out that it can import the types from @emotion/core, but it's much better than before. I'm talking to the maintainers on Discord about whether TypeScript can be updated to 4.2 to avoid having to declare the types.

@Methuselah96
Copy link
Contributor Author

@ndelangen Merge conflicts resolved.

@shilman shilman merged commit ab2e245 into storybookjs:next Dec 10, 2021
@Methuselah96 Methuselah96 deleted the fix-emotion-dependencies branch December 10, 2021 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants