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

chore!: Update @elastic/eui and @emotion/react in Feast UI #4597

Merged
merged 2 commits into from
Oct 6, 2024

Conversation

peruukki
Copy link
Contributor

@peruukki peruukki commented Oct 3, 2024

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
on-demand-feature-view-before
After
on-demand-feature-view-after

More before/after screenshots

Landing page

Before
landing-page-before
After
landing-page-after

Project overview

Before
project-overview-before
After
project-overview-after

Loading state

Before
loading-state-before
After
loading-state-after

Data source

Before
data-source-before
After
data-source-after

Feature service

Before
feature-service-before
After
feature-service-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.

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]>
Copy link
Member

@franciscojavierarceo franciscojavierarceo left a 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.

@franciscojavierarceo franciscojavierarceo enabled auto-merge (squash) October 3, 2024 12:26
@franciscojavierarceo franciscojavierarceo merged commit b9ddbf9 into feast-dev:master Oct 6, 2024
23 of 24 checks passed
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.

3 participants