fix(EmptyState): illustrations should render correctly now

[matthewgallo]

Contributes to #324

This PR fixes the asset loading issue in the EmptyState component because of how we were loading the illustration. Thanks @davidmenendez for suggestion some solutions to this issue. I tried webpack's require.context however, it did not seem to be working within storybook so I went with the approach of importing the assets from a separate file.

UPDATE (03/02/21):
The <EmptyState /> component is now broken up into several components so we avoid the issue of importing every illustration until we can get dynamic imports working.

Changelog

New
packages/experimental/src/components/EmptyState/EmptyStateIllustrations.js

Changed
packages/experimental/src/components/EmptyState/EmptyState.js
packages/experimental/src/components/EmptyState/EmptyState.stories.js

[netlify]

Deploy preview for ibm-cloud-cognitive ready!

Built with commit 549e273

https://deploy-preview-332--ibm-cloud-cognitive.netlify.app

[davidmenendez]

Approved, but could you do me a favor and just add like a comment or something to the top of the EmptyIllustrations.js file and just say something like note to self- find better solution because i feel like there has to be a better solution or there will be one one day and i'd like to have it documented somewhere as a reminder. but i think this is a good start for now 👍

[areddon]

This will result in all SVGs to be loaded by the consumer without the possibility of tree shaking. This is something that Carbon has already solved with the use of Icons and I am sure that there is a pattern to be reused.

[davidmenendez]

This will result in all SVGs to be loaded by the consumer without the possibility of tree shaking. This is something that Carbon has already solved with the use of Icons and I am sure that there is a pattern to be reused.

if that's the case i'll go ahead i'll go ahead and remove my approval. i'm glad to hear this has been figured out already. i don't know enough about storybook to have had any better solutions in mind. @matthewgallo you should reach out to the carbon team to get their input on this and either update this PR or open up a new one. @areddon thank you for the heads up 👍

new information has come to my attention that should lead to a better implementation

[areddon]

Carbon does this by having the package icons-react export a React component for each icon SVG in their icons package. Then various React components in their react package such as Button accept a renderIcon prop to draw the SVG and allow for proper tree shaking.

[SimonFinney]

Carbon does this by having the package icons-react export a React component for each icon SVG in their icons package. Then various React components in their react package such as Button accept a renderIcon prop to draw the SVG and allow for proper tree shaking.

@areddon Good point - How does this work for components like this where the illustrations are internal and predetermined based on consumer selection? Wouldn't this be unavoidable unless they're conditionally imported, or is the nature of an SVG-as-a-module what plays nice with tree-shaking?

/cc @joshblack Would you have some additional context here?

[areddon]

I think that the point of these components as building blocks is to be as lean as possible and push those decisions down to the consumer. We can decide whether this library exports a React component for each SVG or that is done by a separate package for all of them. Probably easier to maintain if it is a separate library like icons-react as it allows other developers to just drop an SVG in there. Since the code for the EmptyState is rather straight forward, we could have it constructed so that it could be consumed like the example below.

import { error } from 'pal-icons-react/dark'

<EmptyState renderIcon={error} />

In most cases, the illustration to use will be known ahead of time and if not, then it is left up to the consumer regarding how they want handle conditional cases. They would either load multiple and use conditional rendering or use dynamic imports, but the end result is that we give them the choice.

I see that we also have SVGs for both light and dark, is it not possible for us to instead have a single SVG that is modified via styles? Otherwise at the very least the consumer will have to load both or using dynamic imports for the correct one.

[davidmenendez]

I'm personally not a fan of this because I think it's antithetical to what we're trying to do. We're trying to build out these components with specific use cases in mind to make it easier for the consumer. It should be as easy as <EmptyState illustration="notfound" /> instead of having the user have to import all the illustrations and set the props themselves.

I'm ok with accepting that a better solution for this just hasn't come up yet. I just wanted to express my concern with this. We can't really compare this with carbon when we're building specific flows and specific use cases so that the user doesn't have to.

[SimonFinney]

I'm personally not a fan of this because I think it's antithetical to what we're trying to do. We're trying to build out these components with specific use cases in mind to make it easier for the consumer. It should be as easy as <EmptyState illustration="notfound" /> instead of having the user have to import all the illustrations and set the props themselves.

I'm ok with accepting that a better solution for this just hasn't come up yet. I just wanted to express my concern with this. We can't really compare this with carbon when we're building specific flows and specific use cases so that the user doesn't have to.

I agree, and I wonder what the middle-ground is here between enforcing illustration consistency and being conscious of bundle size. The renderIcon approach leaves it vulnerable to abuse, but that could happen anyway given the component already supports custom illustrations.

[davidmenendez]

@matthewgallo can you explain why require.context wasn't working? seems like it might be a solution, but i understand storybook might be causing issues https://devdesigner.xyz/dynamic-image-import-with-create-react-app/read

[matthewgallo]

Sure @davidmenendez, when I tried using webpack's require.context I kept getting the following error, require.context is not a function seems related to the following issue. I attempted to change some of our babel settings but wasn't able to get it working.

[areddon]

I don't think that the solution I proposed is rather difficult for consumers to use and they would be familiar with it as it is the same for Carbon and Icons. It can be taken a step further such that we export a component per illustration. <NotFoundEmptyState />, <EmptyStateNotFound /> or <NotFound />

An implementation as is in this PR becomes a breaking change in the future and will only result in bundle size increases as more SVGs get added. (Not just for empty states but other usage of SVGs)

[davidmenendez]

@matthewgallo that's strange because i'm not seeing that locally 🤔

here's a refactor i did that's working

  const requestImageFile = require.context('./assets', true, /.svg$/);
  const defaultIllustrationOptions = [
    'nodata',
    'error',
    'unauthorized',
    'notags',
    'notfound',
    'notifications',
  ];
  const src = typeof illustration === 'string' && defaultIllustrationOptions.includes(illustration)
    ? requestImageFile(`./${illustrationTheme}/${illustration}.svg`)
    : illustration;
  const classes = cx([
    `${pkgPrefix}-empty-state-illustration`,
    `${pkgPrefix}-empty-state-illustration--${illustrationSize}`,
  ]);
...
<img src={src} alt="Empty state illustration" className={classes} />

[matthewgallo]

Right, I see your point @areddon. But I also agree with @SimonFinney and @davidmenendez in that these component designs have strict guidance as to which illustrations should be used and also that these illustrations aren't meant to be used as part of an icon library. Maybe we should just look into getting require.context working, then we can just be able to use the dynamic import.

[areddon]

Neither implementation has an impact on their usage, only how they are bundled and consumed. We also don't have to move the SVGs to a separate package if we were to create a component for each illustration.

[matthewgallo]

@davidmenendez that was able to work for me too. Maybe the issue was that I was using require.context within the return of this component. I remove the extra file that imported all of the illustrations and now this uses a dynamic import.

[joshblack]

@SimonFinney I think what was explained totally nails it, in the past we ran into problems with the <Icon name="icon-name"> pattern and tree-shaking. In that case, the dynamic lookup from a module leads to the whole thing being included. This resulted in the component-per-icon strategy and the renderIcon prop (which tends to be called icon in other systems, it seems).

Those types of props are definitely open-ended, and if there was ever a situation to limit them to only a specific set we might try and approach it through either prop type validation, making components-per-variant, or adding a variant prop to specify which icon to use.

[davidmenendez]

@joshblack @areddon I think you guys have a better understanding of tree shaking and webpack optimization than me. thoughts on using require.context in this scenario?

[joshblack]

@davidmenendez I think require.context can definitely work if the project knows that downstream users can leverage it correctly (like with webpack picking up .svg files). On our we tend to avoid bundler-specific conventions but this definitely can cause headaches for us 😂

Separately, not sure how helpful this is but I did make a Terser sandbox for testing code snippets for tree shaking. It helped out a ton for stuff like: carbon-design-system/carbon#5442 when we ran into weird tree shaking problems. Wasn't sure if it'd help out a ton for this conversation but figured I'd share just in case!

[SimonFinney]

@SimonFinney I think what was explained totally nails it, in the past we ran into problems with the <Icon name="icon-name"> pattern and tree-shaking. In that case, the dynamic lookup from a module leads to the whole thing being included. This resulted in the component-per-icon strategy and the renderIcon prop (which tends to be called icon in other systems, it seems).

Those types of props are definitely open-ended, and if there was ever a situation to limit them to only a specific set we might try and approach it through either prop type validation, making components-per-variant, or adding a variant prop to specify which icon to use.

The variant prop sounds closest to the current implementation, but that's still packaging up 12 SVGs (6 if theming can be delegated within the SVG itself) which is not quite on par with Carbon's iconography assets but based on the discussion still a cause for concern, especially if the number grows.

Maybe prop type validation is enough to deter folks from abusing the open-ended nature of the prop, and as an added bonus, we wouldn't have to worry about locking people into webpack features with require.context.

[dcwarwick]

There are no doubt some bits and pieces here we may want to come back and discuss, but in general this looks good and I'm happy to approve. The only thing is, you'll need to merge in the changes to { pkg }, which will apply to each of the variants here, for it to work with current master.