GeezerVison is showing github.com — chore!: Update @elastic/eui and @emotion/react in Feast UI by peruukki · Pull Request #4597 · feast-dev/feast
Conversation
peruukki
commented
Oct 3, 2024
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
What this PR does / why we need it:
Updates the Elastic UI framework to almost its latest version 95.12.0. The current version 55.0.1 is over 2 years old and doesn't support React 18, which the latest versions do. Good to update its peer dependency @emotion/react to the latest version at the same time.
Note: 95.12.0 was the latest version when I started doing the upgrade, but I noticed version 96 was just released the other day. But when I tried updating to that, I got some mysterious TypeScript errors and decided to leave it; this has already been quite an effort so I just want to get it done. 😅
The UI looks mostly the same as before. The most notable change is that the main content panel now stretches to the bottom of the page. It's possible to avoid that, but it's the default and personally I prefer it. There are some changes in the spacing too, and in how the tag-like elements look. I tried checking all possible pages, but please click around and report anything that looks off.
One notable difference: In on-demand feature views, we have title elements that are also links, and they lost their blue link color in the update due to it being overridden with the title styles. Overriding that would require some extra effort with theme providers etc., and is doable but might not be worth the effort. Here's what I'm talking about:
Before/after screenshots of on-demand feature view
Before
After
More before/after screenshots
Landing page
Before
After
Project overview
Before
After
Loading state
Before
After
Data source
Before
After
Feature service
Before
After
Which issue(s) this PR fixes:
There's a related issue #3784 about upgrading to React 18, this is one step towards enabling it.
Misc
This is a potentially breaking change: If you are using @feast-dev/feast-ui as module in a React app that also uses @elastic/eui, you should update it to a compatible version. If you use @elastic/eui components that have been renamed or replaced with others, you'll need to update your code accordingly. I tested this with my example React app, and didn't notice any problems with the new version.
There are many kinds of changes, so below is a summary to help with the review. I took initial inspiration from elastic/kibana#161418 and then went through the Elastic UI docs when needed.
Other dependency changes
- @emotion/css is now a peer dependency of @elastic/eui, so we need to add it to our dependencies.
- prop-types is no longer a peer dependency of @elastic/eui, so we can remove it from our dependencies.
- typescript needed an upgrade for TypeScript compilation to work in the
build:lib script; it failed due to a syntax error in @types/lodash (dependency of @elastic/eui). Unfortunately this typescript version isn't within the version range of @elastic/eui's peer dependencies, but that one seems overly strict, especially since this version seems to work. Unfortunately this also introduces a temporarily-visible warning in the yarn start output about the typescript version being newer than what ESLint supports, but again, everything seems fine. And to be honest, I don't know what else to do, this has been quite a challenge to get somehow working. 😓
Code changes
- EuiLoadingContent has been replaced with EuiSkeletonText.
- EuiPageContent and EuiPageContentBody have been replaced with EuiPageTemplate, EuiPageTemplate.Header and EuiPageTemplate.Section.
- EuiSideNav no longer takes a style prop, so it is dropped; the width seems fine without it.
- EuiBasicTable now requires the field prop in its columns, and only takes objects in its items.
- The
panelled prop has been moved from Layout's EuiPageBody to each page's EuiPageTemplate, so that the page template's header gets the wanted background color.
- The
sticky prop passed to Layout's EuiPageSidebar needs to be an object with an offset, otherwise the offset is read from --euiFixedHeadersOffset that is unset if there is no fixed EuiHeader on the page.
- Icons: Static class names no longer work for proper styling, we need to pass at least the className prop to the svg element. Passing all props allows possible other props to work too. Also, we no longer need separate components for differently sized icons (16, 32).
- Some overview tab contents are wrapped in an additional EuiFlexGroup to add gaps between sections; they previously appeared through some component margins but not anymore.
- Jest failed to parse chroma-js sources, probably something to do with it being an ES module (its support in Jest is limited: https://jest-archive-august-2023.netlify.app/docs/27.x/ecmascript-modules), so we use the build version of chroma-js with Jest, similarly to d3.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This version of @elastic/eui supports React 18. Good to update
@emotion/react to the latest version at the same time.
Other dependency changes:
- @emotion/css is now a peer dependency of @elastic/eui, so we need to
add it to our dependencies.
- prop-types is no longer a peer dependency of @elastic/eui, so we can
remove it from our dependencies.
- typescript needed an upgrade for TypeScript compilation to work in
the `build:lib` script; it failed due to a syntax error in
@types/lodash (dependency of @elastic/eui). Unfortunately this
typescript version isn't within the version range of @elastic/eui's
peer dependencies, but that one seems overly strict, especially since
this version seems to work. Unfortunately this also introduces a
warning in the `yarn start` about the typescript version being newer
than what ESLint support, but again, everything seems fine. And to be
honest, I don't know what else to do, this has been quite a challenge
to get somehow working. :D
Code changes:
- EuiLoadingContent has been replaced with EuiSkeletonText.
- EuiPageContent and EuiPageContentBody have been replaced with
EuiPageTemplate, EuiPageTemplate.Header and EuiPageTemplate.Section.
- EuiSideNav no longer takes a style prop, so it is dropped; the width
seems fine without it.
- EuiBasicTable now requires the field prop in its columns, and only
takes objects in its items.
- The `panelled` prop has been moved from Layout's EuiPageBody to each
page's EuiPageTemplate, so that the page template's header gets the
wanted background color.
- The `sticky` prop passed to Layout's EuiPageSidebar needs to be an
object with an offset, otherwise the offset is read from
--euiFixedHeadersOffset that is unset if there is no fixed EuiHeader
on the page.
- Icons: Static class names no longer work for proper styling, we need
to pass at least the className prop to the svg element. Passing all
props allows possible other props to work too. Also, we no longer need
separate components for differently sized icons (16, 32).
- Some overview tab contents are wrapped in an additional EuiFlexGroup
to add gaps between sections; they previously appeared through some
component margins but not anymore.
- Jest failed to parse chroma-js sources, probably something to do with
it being an ES module (its support in Jest is limited:
https://jest-archive-august-2023.netlify.app/docs/27.x/ecmascript-modules),
so we use the build version of chroma-js with Jest, similarly to d3.
BREAKING CHANGE: Consuming apps that use @elastic/eui should update it
to a compatible version. If you use @elastic/eui components that have
been renamed or replaced with others, you'll need to update your code
accordingly.
Signed-off-by: Harri Lehtola <[email protected]>
Node 17 is not an LTS (long-term support) version and apparently
rejected by the latest versions of Elastic UI:
> error @elastic/[email protected]: The engine "node" is incompatible with
> this module. Expected version "16.x || 18.x || >=20.x". Got "17.9.1"
Let's try with the latest LTS version.
Signed-off-by: Harri Lehtola <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Thank you for doing this work! Amazing. We'll make sure to mention how this could be a breaking change in the release notes.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
peruukki reacted with heart emoji
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|
