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.

Sign up for GitHub

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

Jump to bottom

[Emotion] Add @emotion/css as a dev and peer dependency #6288

Merged
merged 11 commits into from
Oct 5, 2022
Merged

[Emotion] Add @emotion/css as a dev and peer dependency #6288

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bc1be72
Install `@emotion/css` and upgrade all Emotion dependencies to latest…
cee-chen Oct 4, 2022
3399bdf
Convert docs anchor links to `@emotion/css`
cee-chen Oct 4, 2022
41e4ac5
Add `@emotion/css` as a peerDependency
cee-chen Oct 4, 2022
88ac73a
Remove `@emotion/cache` usage from EuiProvider in favor of the defaul…
cee-chen Oct 4, 2022
d08a00b
[OPTIONAL] unrequire `@emotion/cache` as a peerDependency
cee-chen Oct 4, 2022
04a81c6
Convert EuiOverlayMask to `@emotion/css`
cee-chen Oct 4, 2022
ff5aaed
Remove incredibly annoying mounted snapshots
cee-chen Oct 4, 2022
817e387
Merge branch 'main' into emotion-deps
cee-chen Oct 4, 2022
14fdc19
consolidated some yarn.lock entries
chandlerprall Oct 5, 2022
516fe7b
"Fix" type reference to use `@emotion/css` instead of `@emotion/cache`
cee-chen Oct 5, 2022
9531573
Merge branch 'emotion-deps' of github.com:constancecchen/eui into emo…
cee-chen Oct 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,12 @@
"@elastic/charts": "^50.0.1",
"@elastic/datemath": "^5.0.3",
"@elastic/eslint-config-kibana": "^0.15.0",
"@emotion/babel-preset-css-prop": "^11.2.0",
"@emotion/cache": "^11.7.1",
"@emotion/eslint-plugin": "^11.7.0",
"@emotion/jest": "^11.9.0",
"@emotion/react": "^11.9.0",
"@emotion/babel-preset-css-prop": "^11.10.0",
"@emotion/cache": "^11.10.3",
"@emotion/css": "^11.10.0",
"@emotion/eslint-plugin": "^11.10.0",
"@emotion/jest": "^11.10.0",
"@emotion/react": "^11.10.4",
"@pmmmwh/react-refresh-webpack-plugin": "^0.5.3",
"@svgr/core": "5.5.0",
"@svgr/plugin-svgo": "^4.0.3",
Expand Down Expand Up @@ -253,8 +254,8 @@
},
"peerDependencies": {
"@elastic/datemath": "^5.0.2",
"@emotion/cache": "11.x",
"@emotion/react": "11.x",
"@emotion/css": "11.x",
Comment on lines -256 to +258
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thompsongl would love your opinion on this (removing @emotion/cache from our required peer dependencies now that we can use @emotion/css instead - see 88ac73a, d08a00b). I don't feel incredibly strongly about it but I do feel it's a nice to have.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's probably the right call. We're making a breaking change with adding @emotion/css, so might as well be thorough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still use @emotion/cache in a few src locations and need to keep the dependency around

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot read file paths. Only one usage remains (cache_provider.tsx) which is only a type but still maintains that dependency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? We had EmotionCache defined as a type before we declared it as a peer dependency:

https://github.com/elastic/eui/pull/6126/files#diff-a1648a5c54676d1ae54abfef57822d0dd31c82d6109e9b3bd789b2626cc06756

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it ends up in eui.d.ts it needs to be a dependency, otherwise the consuming app's typescript will fail to resolve it

from yesterday's build:

declare module '@elastic/eui/src/components/provider/cache/cache_provider' {
	import { PropsWithChildren } from 'react';
	import { EmotionCache } from '@emotion/cache';
	export interface EuiCacheProviderProps {
	    cache?: false | EmotionCache;
	}
	export const EuiCacheProvider: ({ cache, children, }: PropsWithChildren<EuiCacheProviderProps>) => JSX.Element;

}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change this to reference the typeof cache from @emotion/css... but @emotion/css's create-instance.d.ts is calling @emotion/cache's EmotionCache type def anyway:

https://github.com/emotion-js/emotion/blob/main/packages/css/types/create-instance.d.ts

so there's probably some underlying sub-dependency magic going on here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

516fe7b

the above commit in meme form:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the scooby doo change! I understand it ultimately ends up being the same, it does make the housekeeping a little cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure!!

"@types/react": "^16.9 || ^17.0",
"@types/react-dom": "^16.9 || ^17.0",
"moment": "^2.13.0",
Expand Down
1 change: 1 addition & 0 deletions src-docs/src/components/codesandbox/link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ ${demoContent}
'@elastic/datemath',
'@emotion/cache',
'@emotion/react',
'@emotion/css',
'moment',
'react',
'react-dom',
Expand Down
64 changes: 30 additions & 34 deletions src-docs/src/services/routing/heading_anchor_links.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, { useEffect, useState, useMemo } from 'react';
import { createPortal } from 'react-dom';
import { useLocation } from 'react-router-dom';
import { ClassNames } from '@emotion/react';
import { css } from '@emotion/css';

import { EuiButtonIcon, useEuiTheme, logicalCSS } from '../../../../src';

Expand Down Expand Up @@ -33,43 +33,39 @@ export const useHeadingAnchorLinks = () => {
// to quickly get/copy the heading anchor link
const anchorLinks = useMemo(
() => (
<ClassNames>
{({ css }) => (
<>
{headingNodes.map((heading) => {
const headingCss = css`
&:hover [data-anchor-link] {
opacity: 1;
}
`;
const linkCss = css`
opacity: 0;
${logicalCSS('margin-left', euiTheme.size.s)}
<>
{headingNodes.map((heading) => {
const headingCss = css`
&:hover [data-anchor-link] {
opacity: 1;
}
`;
const linkCss = css`
opacity: 0;
${logicalCSS('margin-left', euiTheme.size.s)}
Comment on lines +38 to +45
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like styles from @emotion/css don't get added to the cache location

Screen Shot 2022-10-05 at 9 45 35 AM

Not sure how big a deal this is considering the limited cases where it'll be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, super interesting. Thanks for catching that!

It looks like we'd need to use a custom createInstance to give it a key (https://emotion.sh/docs/@emotion/css#custom-instances) if we absolutely need to fix their location.

I do agree their uses are generally relatively limited, so it may not have to be something we worry about anytime soon.


&:hover,
&:hover,
&:focus {
opacity: 1;
}
`;
opacity: 1;
}
`;

heading.classList.add(headingCss);
heading.classList.add(headingCss);

return createPortal(
<EuiButtonIcon
data-anchor-link
iconType="link"
color="text"
size="xs"
css={linkCss}
aria-label="Go to heading anchor link"
href={`#${pathname}#${heading.id}`}
/>,
heading
);
})}
</>
)}
</ClassNames>
return createPortal(
<EuiButtonIcon
data-anchor-link
iconType="link"
color="text"
size="xs"
className={linkCss}
aria-label="Go to heading anchor link"
href={`#${pathname}#${heading.id}`}
/>,
heading
);
})}
</>
),
[headingNodes] // eslint-disable-line react-hooks/exhaustive-deps
);
Expand Down
2 changes: 1 addition & 1 deletion src-docs/src/views/guidelines/getting_started/getting_started.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export const GettingStarted = {
<EuiSpacer />
<EuiCodeBlock language="bash" isCopyable fontSize="m">
{
'yarn add @elastic/eui @elastic/datemath @emotion/react @emotion/cache moment prop-types'
'yarn add @elastic/eui @elastic/datemath @emotion/react @emotion/css moment prop-types'
}
</EuiCodeBlock>
<EuiSpacer />
Expand Down
Loading