add display names to components #817

dburles · 2020-03-31T01:33:41Z

React devtools and Emotion will now render the correct component names, for #811

React devtools and Emotion will now render the correct component names

lachlanjc · 2020-03-31T02:40:15Z

This is awesome! Any reason we can’t do it with the simpler syntax though?

dburles · 2020-03-31T03:08:51Z

For that instance it is due to the emotion styled API not reading it for whatever reason

dburles · 2020-03-31T03:11:31Z

The function name approach is needed when using forwardRef https://reactjs.org/docs/forwarding-refs.html#displaying-a-custom-name-in-devtools

jxnblk · 2020-04-01T15:49:12Z

Rad, thanks! Would changing the Box component to not use the styled HOC affect this at all? (See #692 (comment))

dburles · 2020-04-01T23:43:48Z

No it should be okay, if it's just a regular component, it will only need to pass a named function to forwardRef. These changes will definitely conflict with #692. I think if this were merged first, it shouldn't be very difficult to reconcile.

yuriy-sng · 2020-04-03T14:02:49Z

Hey! Thanks for this PR related to my issue.

I may not be approaching this the right way, but when I inspect the components in the Deploy preview all I see are -Box suffixes, instead of -Image, -Heading and such.

dburles · 2020-04-03T22:50:17Z

Hey! Thanks for this PR related to my issue.

I may not be approaching this the right way, but when I inspect the components in the Deploy preview all I see are -Box suffixes, instead of -Image, -Heading and such.

Yeah that is odd, I'll have a look into it

dburles · 2020-04-03T23:43:30Z

Ok after digging into it a bit, I think the best solution to ensure emotion renders the correct names is to add the label property to all components. I'll push a fix soon.

dburles · 2020-04-04T02:15:34Z

Edit: See #823

I'm kinda second guessing using the emotion label option as they're not stripped in production, though it is one solution. I've gone down a bit of a rabbit hole and the indirection through the Box component is causing problems in that emotion always resolves to Box unless we otherwise specify a label on every component.

This should be able to be greatly simplified by writing components without reference to Box such as:

/** @jsx jsx */
import { jsx } from 'theme-ui'
import React from 'react'

export const Alert = React.forwardRef(function Alert(props, ref) {
  return (
    <div
      ref={ref}
      {...props}
      sx={{
        display: 'flex',
        alignItems: 'center',
        px: 3,
        py: 2,
        fontWeight: 'bold',
        color: 'white',
        bg: 'primary',
        borderRadius: 4,
        ...(props.variant && { variant: `alerts.${props.variant}` }),
      }}
    />
  )
})

Some of these components use styled-system style props, which is another reason why they all inherit from the Box component, e.g.:

<Alert variant='secondary' mb={2}>Secondary</Alert>

IMO, there is no need for this now that we have the sx prop, it is odd to have two API's and only adds confusion. This might also simplify the TS refactor over at #692. Also, this definitely plays into the variant prop idea also: #800.

dburles · 2020-04-08T10:00:54Z

I think I'll roll with the label option for now as that'll ensure that emotion renders the correct names. I'll try have that up soon.

dburles · 2020-04-13T06:59:34Z

Alright so I think this is ready! Emotion will now render the component names, it's unfortunate but they'll all be named like css-191ogd4-Box-Text for example (always with Box). So to summarise this PR, it includes two things:

React devtools will now render the component names
Emotion will generate classnames containing the component names

It would be even better if we could get CSS sourcemaps working too, the emotion babel plugin will require changes to support theme-ui. Something to look at in future :)

jxnblk · 2020-04-14T20:40:22Z

packages/components/src/Alert.js

-    }}
-  />
-))
+export const Alert = React.forwardRef(function Alert(props, ref) {


For my understanding: is the named function syntax still necessary with the Emotion label property added below?

Also, holy smokes! I don't think I realized the label property would work more generally in the sx prop – that's probably worth putting in a guide in the docs or mentioning somewhere 🤔

For my understanding: is the named function syntax still necessary with the Emotion label property added below?

Just for React devtools

Does the displayName static property no longer work for React devtools?

It does but just not when you’re using forwardRef https://reactjs.org/docs/forwarding-refs.html#displaying-a-custom-name-in-devtools

Just thought I'd double check it, it does indeed work with forwardRef! Strange, I am sure that it wasn't working at the time, anyway, I'd probably stick with this approach as it's what's recommended and is more concise.

atanasster · 2020-12-10T23:57:03Z

Hi @dburles - this is a great PR.

Can you please upgrade it to resolve conflicts and we also have some new components

Paragraph
Switch

Also whats the final status on displayName?

I hope we can merge this soon, as @lachlanjc is working on a rewrite to remove emotion/styled and it seems there is some overlap here.

vercel · 2020-12-11T05:06:52Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/systemui/theme-ui/20uog2mlc
✅ Preview: https://theme-ui-git-components-display-names.systemui.vercel.app

dburles · 2020-12-11T07:49:12Z

@atanasster Thanks. I've just fixed the merge conflicts. Named functions serve the same purpose as displayName, so either way it doesn't make any difference.

I think we should probably remove the label property:

It isn't stripped in production, which means additional data over the wire.
It's tedious to correctly handle alongside the __css prop on Box. For instance CheckBoxChecked (in Checkbox) which renders the SVG component has custom styles defined using the __css prop. The means in each lower level component (SVG in this case), for those styles to function we would have to merge label with those styles so that the styles are correctly applied. It's possible to do, but unnecessarily tedious. This is only a problem for such instances (which I think are few, but still...).

I'm not sure label is worth the additional complexity, at least with the current implementation with everything using Box.

If we can make a decision on that I am happy to remove it and at least sort out the naming in devtools for now. Emotion label can always be revisited later.

atanasster · 2020-12-11T08:09:02Z

Thanks @dburles thats great

I would say remove label. You are correct this PR should be only about the devTools and we can decide about label in some other PR if its requested by users.

Would you mind switching to static displayName, its a bit more standard and will be also easier for the rework that @lachlanjc is doing.

dburles · 2020-12-11T08:54:40Z

I can't find why displayName appears to be working now (it didn't used to), the React docs still recommend naming the functions instead of using displayName when using forwardRef. So I'm not sure which is incorrect.

atanasster · 2020-12-11T08:57:35Z

@dburles i think displaying the component name looks cleaner too, no?

dburles · 2020-12-11T08:59:47Z

I mean like, technically you shouldn't be able to use displayName on forwardRef components. I do recall it not working when writing this PR some time in the past, now for some reason it seems to work. I'm not sure what that means because the docs still recommend providing a named function to forwardRef (and not displayName). Unless I can confirm that displayName is now supported on forwardRef components I wouldn't recommend it.

atanasster · 2020-12-11T09:05:11Z

I see you are correct it was recommended by bvaughn in that thread. You make the call, seems both work so we should be good.

dburles · 2020-12-11T09:07:50Z

We could do this too, but it's not really as nice:

const Component = ({}, ref) => ...;
Component.displayName = 'Button';
const Button = React.forwardRef(Component);
export default Button;

dburles · 2020-12-11T09:09:44Z

I'll aim to have the label removed by early next week.

atanasster · 2020-12-11T09:12:53Z

Fantastic, really looking forward to have your PR merged.

For displayName - any choice you make is fine by me, but until then (monday) if @hasparus @lachlanjc have any notes about it here

hasparus

Great PR, @dburles! Thank you!

@atanasster let's merge this after the label removal.

atanasster · 2020-12-14T00:52:51Z

@dburles - would you mind updating Paragraph and Switch, just so we dont forget to bring them up to date as well.

dburles · 2020-12-14T08:34:30Z

@atanasster Whoops, nice spot. Just updated those, should be good to go then.

atanasster · 2020-12-14T20:59:21Z

@dburles - thanks a lot - its ready to merge. Let's just wait @hasparus to confirm if we are merging only bug fixes to release a 0.6.0 stable, or if we are also merging features.

atanasster

Thanks a bunch, great PR

…ay-names

hasparus · 2020-12-16T12:58:37Z

Let's just wait @hasparus to confirm if we are merging only bug fixes to release a 0.6.0 stable, or if we are also merging features.

You wrote it like I'm some sort of authority.

This PR is open since March, and IMHO it's a bugfix, as I'd expect proper names in React DevTools.

hasparus · 2020-12-16T12:59:21Z

Sorry it took so long to merge it, @dburles 🙏

add display names to components

1ae38e1

React devtools and Emotion will now render the correct component names

dburles mentioned this pull request Apr 4, 2020

New variant API #823

Closed

7 tasks

dburles added 2 commits April 13, 2020 16:44

add emotion label property to components

e2c5454

appears to be unavoidable

ecb9cdf

jxnblk reviewed Apr 14, 2020

View reviewed changes

Merge branch 'master' into components-display-names

2885f38

hasparus mentioned this pull request Jun 23, 2020

Convert to TypeScript #668

Closed

32 tasks

dburles mentioned this pull request Aug 31, 2020

Improve Components DisplayNames properties #1080

Closed

lachlanjc linked an issue Dec 3, 2020 that may be closed by this pull request

Improve Components DisplayNames properties #1080

Closed

lachlanjc added the enhancement New feature or request label Dec 3, 2020

lachlanjc assigned atanasster Dec 6, 2020

Merge branch 'master' into components-display-names

821f51b

dburles requested review from atanasster, hasparus and lachlanjc as code owners December 11, 2020 05:06

vercel bot had a problem deploying to Preview December 11, 2020 05:06 Failure

optional

422364e

vercel bot deployed to Preview December 11, 2020 07:18 View deployment

hasparus approved these changes Dec 12, 2020

View reviewed changes

dburles added 2 commits December 14, 2020 09:50

remove label

0a7e75e

add displayName to non-forwardRef components

d4a66d6

vercel bot deployed to Preview December 13, 2020 22:54 View deployment

add function names to Switch and Paragraph

18349e2

vercel bot deployed to Preview December 14, 2020 08:34 View deployment

hasparus closed this Dec 14, 2020

hasparus deleted the branch system-ui:develop December 14, 2020 08:53

hasparus reopened this Dec 14, 2020

hasparus changed the base branch from master to develop December 14, 2020 09:12

atanasster approved these changes Dec 14, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into components-displ…

95f6ea8

…ay-names

vercel bot deployed to Preview December 14, 2020 21:52 View deployment

hasparus merged commit e9429e6 into system-ui:develop Dec 16, 2020

add display names to components #817

add display names to components #817

Conversation

dburles commented Mar 31, 2020

lachlanjc commented Mar 31, 2020 • edited Loading

dburles commented Mar 31, 2020

dburles commented Mar 31, 2020 • edited Loading

jxnblk commented Apr 1, 2020

dburles commented Apr 1, 2020

yuriy-sng commented Apr 3, 2020 • edited Loading

dburles commented Apr 3, 2020

dburles commented Apr 3, 2020

dburles commented Apr 4, 2020 • edited Loading

dburles commented Apr 8, 2020

dburles commented Apr 13, 2020 • edited Loading

jxnblk Apr 14, 2020

Choose a reason for hiding this comment

jxnblk Apr 14, 2020

Choose a reason for hiding this comment

dburles Apr 14, 2020

Choose a reason for hiding this comment

jxnblk Apr 15, 2020

Choose a reason for hiding this comment

dburles Apr 15, 2020

Choose a reason for hiding this comment

dburles May 7, 2020

Choose a reason for hiding this comment

atanasster commented Dec 10, 2020

vercel bot commented Dec 11, 2020 • edited Loading

dburles commented Dec 11, 2020

atanasster commented Dec 11, 2020

dburles commented Dec 11, 2020

atanasster commented Dec 11, 2020

dburles commented Dec 11, 2020 • edited Loading

atanasster commented Dec 11, 2020

dburles commented Dec 11, 2020

dburles commented Dec 11, 2020

atanasster commented Dec 11, 2020

hasparus left a comment

Choose a reason for hiding this comment

atanasster commented Dec 14, 2020

dburles commented Dec 14, 2020

atanasster commented Dec 14, 2020

atanasster left a comment

Choose a reason for hiding this comment

hasparus commented Dec 16, 2020

hasparus commented Dec 16, 2020

lachlanjc commented Mar 31, 2020 •

edited

Loading

dburles commented Mar 31, 2020 •

edited

Loading

yuriy-sng commented Apr 3, 2020 •

edited

Loading

dburles commented Apr 4, 2020 •

edited

Loading

dburles commented Apr 13, 2020 •

edited

Loading

vercel bot commented Dec 11, 2020 •

edited

Loading

dburles commented Dec 11, 2020 •

edited

Loading