SelectDropdown: cleanup code and migrate CSS to webpack

[jsnajdr]

The SelectDropdown component accumulated a lot of cruft over the years, so I did a comprehensive cleanup as part of the CSS migration.

Please review commit by commit, I tried to keep them very small and explain everything.

Similar to SegmentedControl in #35051, I put the subcomponents as static properties on the main component:

<SelectDropdown>
  <SelectDropdown.Item />
  <SelectDropdown.Label />
  <SelectDropdown.Separator />
</SelectDropdown>

[matticbot]

Test live: https://calypso.live/?branch=migrate/select-dropdown-css

[sgomes]

FYI @jsnajdr: I have an open PR atm which is touching the same component: #34989

[sgomes]

@jsnajdr It looks like your PR supersedes mine, so I can probably close it. What do you think? Looks like we were working on the same thing at the same time (although you went a heck of a lot further 😄)

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~49 bytes removed 📉 [gzipped])

name                   parsed_size           gzip_size
entry-main                 -3128 B  (-0.2%)      -64 B  (-0.0%)
entry-domains-landing       -477 B  (-0.1%)      +15 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legacy SCSS Stylesheet (~699 bytes removed 📉 [gzipped])

name       parsed_size           gzip_size
style.css      -5362 B  (-3.8%)     -699 B  (-3.1%)

The monolithic CSS stylesheet that is downloaded on every app load.
👍 Thanks for making the stylesheet smaller in this PR!

Sections (~13720 bytes removed 📉 [gzipped])

name                   parsed_size           gzip_size
plans                      -5538 B  (-1.0%)     -700 B  (-0.5%)
checkout                   -4584 B  (-0.5%)     -778 B  (-0.4%)
purchases                  -3541 B  (-0.4%)     -605 B  (-0.3%)
theme                      -3472 B  (-1.0%)     -556 B  (-0.6%)
woocommerce                -3471 B  (-0.2%)     -375 B  (-0.1%)
settings                   -3471 B  (-0.6%)     -376 B  (-0.3%)
marketing                  -3471 B  (-0.4%)     -375 B  (-0.2%)
plugins                    -3349 B  (-0.6%)     -437 B  (-0.3%)
help                       -2922 B  (-0.5%)     -398 B  (-0.3%)
email                      -2920 B  (-0.8%)     -383 B  (-0.4%)
domains                    -2920 B  (-0.3%)     -383 B  (-0.2%)
jetpack-connect            -2552 B  (-0.4%)     -194 B  (-0.1%)
post-editor                -2463 B  (-0.1%)     -363 B  (-0.1%)
stats                      -2375 B  (-0.3%)     -499 B  (-0.3%)
settings-writing           -2375 B  (-0.5%)     -354 B  (-0.3%)
settings-security          -2375 B  (-1.1%)     -356 B  (-0.6%)
settings-performance       -2375 B  (-1.1%)     -345 B  (-0.6%)
settings-discussion        -2375 B  (-1.3%)     -361 B  (-0.8%)
comments                   -2375 B  (-0.4%)     -359 B  (-0.3%)
gutenberg-editor           -2352 B  (-0.3%)     -355 B  (-0.2%)
posts-pages                -2299 B  (-0.7%)     -381 B  (-0.5%)
signup                     -2061 B  (-0.8%)     -178 B  (-0.3%)
wp-super-cache             -1824 B  (-0.8%)     -346 B  (-0.6%)
security                   -1824 B  (-0.4%)     -336 B  (-0.3%)
reader                     -1824 B  (-0.5%)     -328 B  (-0.3%)
posts-custom               -1824 B  (-0.6%)     -338 B  (-0.5%)
people                     -1824 B  (-0.5%)     -349 B  (-0.4%)
notification-settings      -1824 B  (-0.6%)     -338 B  (-0.4%)
media                      -1824 B  (-0.5%)     -340 B  (-0.3%)
google-my-business         -1824 B  (-0.6%)     -337 B  (-0.4%)
earn                       -1824 B  (-0.7%)     -342 B  (-0.5%)
account                    -1824 B  (-0.5%)     -348 B  (-0.4%)
zoninator                  -1777 B  (-0.8%)     -314 B  (-0.6%)
themes                     -1648 B  (-0.4%)     -181 B  (-0.2%)
export                     -1096 B  (-0.4%)      -33 B  (-0.0%)
customize                  -1096 B  (-0.4%)      -33 B  (-0.0%)
account-close              -1096 B  (-0.3%)      -33 B  (-0.0%)
login                       -975 B  (-0.7%)     -287 B  (-0.8%)
import                      -553 B  (-0.3%)     -163 B  (-0.3%)
activity                    -551 B  (-0.1%)     -163 B  (-0.1%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~3612 bytes removed 📉 [gzipped])

name                                                         parsed_size           gzip_size
async-load-design-blocks                                         -4496 B  (-0.2%)     -599 B  (-0.1%)
async-load-design                                                -2957 B  (-0.2%)     -389 B  (-0.1%)
async-load-blocks-inline-help-popover                            -2922 B  (-0.9%)     -401 B  (-0.5%)
async-load-design-playground                                     -1909 B  (-0.1%)     -363 B  (-0.1%)
async-load-components-web-preview-component                      -1827 B  (-0.5%)     -332 B  (-0.4%)
async-load-quick-language-switcher                               -1824 B  (-6.0%)     -361 B  (-4.3%)
async-load-signup-steps-plans                                    -1436 B  (-1.0%)     -320 B  (-0.8%)
async-load-my-sites-current-site-stale-cart-items-notice         -1096 B  (-0.9%)      -33 B  (-0.1%)
async-load-my-sites-current-site-notice                          -1096 B  (-0.7%)      -33 B  (-0.1%)
async-load-my-sites-checklist-wpcom-checklist-component-jsx      -1029 B  (-0.7%)      +53 B  (+0.2%)
async-load-signup-steps-plans-atomic-store                        -955 B  (-0.9%)     -330 B  (-1.1%)
async-load-signup-steps-import-url-onboarding                     -553 B  (-1.4%)     -163 B  (-1.4%)
async-load-signup-steps-import-url                                -553 B  (-2.1%)       -4 B  (-0.1%)
async-load-signup-steps-clone-point                               -551 B  (-0.3%)     -163 B  (-0.4%)
async-load-blocks-calendar-popover                                -551 B  (-0.2%)      -11 B  (-0.0%)
async-load-signup-steps-clone-destination                         -482 B  (-2.4%)      -12 B  (-0.2%)
async-load-signup-steps-user                                      -478 B  (-0.5%)     -148 B  (-0.6%)
async-load-post-editor-media-modal                                 -50 B  (-0.0%)       -3 B  (-0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[jsnajdr]

It looks like your PR supersedes mine, so I can probably close it.

I wanted to just migrate the CSS, but then got absorbed by making a lot if improvements in 22 commits 😄

Looking at your PR, I'm doing a few things differently:

• I don't clear the this.itemRefs array on each render. Maybe I should do it?
• I don't check if a ref.current is set before calling focus() on it. I think they are always guaranteed to be set. They are accessed inside DOM handlers, which means the respective elements are always present.
• to focus the link inside the item, I created a focusLink instance method. There's a chain of two refs in action.

[sgomes]

Looking at your PR, I'm doing a few things differently:

I think your changes are generally better.

• I don't clear the this.itemRefs array on each render. Maybe I should do it?

The reason I did this was to avoid keeping around references for longer than they should exist. If option n disappears and the total number of elements goes down, then it may remain in the array while the other indices get reassigned. It's probably not an issue at all in practice, but I figured I'd play it safe.

• I don't check if a ref.current is set before calling focus() on it. I think they are always guaranteed to be set. They are accessed inside DOM handlers, which means the respective elements are always present.

Right, I tend to just write that type of safeguard mechanically without much thought. I think it's probably safe here, yes.

• to focus the link inside the item, I created a focusLink instance method. There's a chain of two refs in action.

Yes, that's probably a better model than the forwardRef I was using.

[diegohaz]

I'm pretty sure we loose tree shaking when doing Dropdown.Label instead of DropdownLabel. This may not be a huge problem for this component as this is really small, but I think it's worth keeping that in mind for other components.

An alternative solution that removes the deep imports and keeps tree shaking working is just re-exporting them in the index file and importing them separately from the same path:

import { DropdownLabel, DropdownSeparator } from 'components/select-dropdown';

Regarding ref.current, it'll be undefined if the dom element is removed for some reason (e.g. false && <div />). Since it's being used inside event handlers, maybe event.currentTarget can be used instead.

EDIT: Looks like most of the event handlers are on child elements, so event.currentTarget wouldn't be useful.

[jsnajdr]

I'm pretty sure we loose tree shaking when doing Dropdown.Label instead of DropdownLabel.

We already don't have any tree shaking for this component, because Label and Separator are used when passing an options array:

<SelectDropdown options = { [
  { label: 'Unclickable', isLabel: true }, // this will be a SelectDropdown.Label
  null, // this will be a SelectDropdown.Separator
  { label: 'Real Option', value: 'opt' }, // this will be SelectDropdown.Item
] } />

Adding the subcomponents as static fields doesn't add any new dependencies: there already are there.

Regarding ref.current, it'll be undefined if the dom element is removed for some reason

In our case though, the container is always rendered (no conditional rendering is there) and the Item refs point to the component class instance, not its DOM elements.

[jsnajdr]

I just rebased the branch onto latest master (I was a bit behind) and update all the unit tests.

[diegohaz]

We already don't have any tree shaking for this component, because Label and Separator are used when passing an options array. Adding the subcomponents as static fields doesn't add any new dependencies: there already are there.

Ah, yeah! I missed that! I still wonder about consistency: instead of adopting ParentChild and Parent.Child for different components depending on whether they support tree shaking, isn't it better to follow one standard (ParentChild) since it works regardless? I don't know if it was discussed before.

I think I started checking all ref.currents just because TypeScript complained about it (and often times it was right).

[diegohaz]

Not something that should be fixed in this PR or any time soon, but I think it's worth noting. If we're ever going to render this component on the server, this can potentially break since this instances will be shared between all clients accessing the same server instance.

I guess the best solution we have now is managing IDs with React Context, but this might break in the future with React Async mode. People have been discussing alternatives here: reactjs/rfcs#32

[jsnajdr]

That's a good point: the generated IDs will be different on server and client. Thanks for linking that React issue! As most other people who commented there, all we need the ID for is to link elements together with various ARIA attributes.

I think managing IDs with React Context would solve the issue even in Async mode. I don't immediately see how Async mode could break it. Initial rendering on server and client is always sync and only the later updates can be async, preempt and cancel each other etc.

[jsnajdr]

I still wonder about consistency: instead of adopting ParentChild and Parent.Child for different components depending on whether they support tree shaking, isn't it better to follow one standard (ParentChild) since it works regardless? I don't know if it was discussed before.

I'm proposing the Parent.Child convention only for components that are part of the same module and are always used together, like ul and li or select and option.

The goal is to improve the DX when importing the component. Some context:

This convention is simply bad:

import Dropdown from 'components/dropdown';
import DropdownItem from 'components/dropdown/item';

because it can't be easily migrated to an import from package:

import Dropdown from '@automattic/calypso-ui/dropdown';
import DropdownItem from '@automattic/calypso-ui/dropdown/item';

When importing from package subdirectories like this, it's impossible to publish multiple versions of the bundle (CJS, ESM, ES:next, ...) because the main and module fields in package.json will be ignored.

We always need to import from the root package (where the index.js contains reexports and has sideEffects: false) and then we have two options:

import { Dropdown, DropdownItem } from '@automattic/calypso-ui';

<Dropdown>
  <DropdownItem />
</Dropdown>

or

import { Dropdown } from '@automattic/calypso-ui';

<Dropdown>
  <Dropdown.Item />
</Dropdown>

I like the second option much better: I import only one thing from the package. Because conceptually it's one thing with several different parts rather than two different things.

Does that make sense?

[diegohaz]

When importing from package subdirectories like this, it's impossible to publish multiple versions of the bundle (CJS, ESM, ES:next, ...) because the main and module fields in package.json will be ignored.

It's possible! But it requires some additional build script to create proxy directories. Those directories include only a package.json pointing to the canonical sources, like this:

With this approach, users are able to import either from the root or from the public subdirectories without losing anything, as explained here: https://reakit.io/docs/bundle-size/

Ideally, build tools like Babel and Webpack should be smart enough and people shouldn't be worrying about things like that (maybe Prepack will save us in the future?). But, until this becomes a reality, importing from subdirectories is usually useful for libraries and apps without tree shaking support. Plus, from what I've seen, people tend to adopt a library more easily if they don't feel like they're including the whole thing when they only need a piece.

Regarding importing separately or not, I used to prefer what you're suggesting (Dropdown.Item). I agree that it has better DX. But recently I've migrated to the separate component/file since you can create bigger modules (e.g. Form) with child components that will not necessarily be used all the time without worrying about unnecessarily increasing bundle size.

It's up to the user if they want the convenience of importing everything as an object:

import * as d from "reakit/Dialog";

// not pretty, but tradeoffs 🤷‍♂️
<d.DialogBackdrop />
<d.DialogDisclosure />
<d.Dialog />

Or if they want it separate:

import { Dialog, DialogBackdrop } from "reakit";
import { Dialog, DialogBackdrop } from "reakit/Dialog";
import { Dialog } from "reakit/Dialog/Dialog";
import { DialogBackdrop } from "reakit/Dialog/DialogBackdrop";

[jsnajdr]

It's possible! But it requires some additional build script to create proxy directories.

Thanks, I didn't realize that! Every component can have its own package.json with the right pointers.

If I only converted:

import SelectDropdown from 'components/select-dropdown';
import SelectDropdownItem from 'components/select-dropdown/item';

to

import { SelectDropdown, SelectDropdownItem } from 'components/select-dropdown';

in this PR and avoided the controversial SelectDropdown.Item convention, would that be better?

The main objective of this PR is to migrate CSS, not to rearrange the imports. So I'm willing to give up that part completely if there's not 100% consensus that it's a good idea 🙂

[jsnajdr]

I think I started checking all ref.currents just because TypeScript complained about it (and often times it was right).

Consider a simple component like this:

class ReffyComponent extends Component {
  inputRef = createRef();

  componentDidMount() {
    this.inputRef.current.focus();
  }

  render() {
    return (
      <div>
        <input type="text" ref={ this.inputRef } />
      </div>
    );
  }
}

At the time when componentDidMount is called, this.inputRef.current is guaranteed to be a valid DOM element object. At other times during the render cycle it's not always so. The ref is initially undefined and is set to null and then back to DOM object during a rerender, but at the time when componentDidMount or componentDidUpdate is called, it's always a DOM object.

We know that because we know how React works. And I understand that TypeScript doesn't know that. But in that case TypeScript becomes a liability and doesn't add value. Is there any way how to tell TypeScript that guarding this.inputRef.current is not necessary?

[diegohaz]

Is there any way how to tell TypeScript that guarding this.inputRef.current is not necessary?

We can do this.inputRef.current!.focus() or just // @ts-ignore it. I guess the problem that TS is concerned about is that the component may be something like this:

<div>
  { loading ? 'Loading...' : <input type="text" ref={ this.inputRef } /> }
</div>

But, guarding it could make it harder to find the error if this component is updated to something like that in the future, so I vote for keeping it as is. :)

I think the PR is okay, I just wanted to kick off a discussion around creating a consistent pattern around modules/imports, but this can be discussed in other issue/PR.

Are the failing checks on the PR a blocker?

[jsnajdr]

I guess the problem that TS is concerned about is that the component may be something like this:

Yes, in that case, the ref is not always set. But TypeScript doesn't distinguish between the two cases and its advice is not very helpful.

Also, when showing a placeholder like in your example, the lifecycle method is likely to be similar to this:

componentDidMount() {
  if ( this.props.loading ) {
    // do some magic on the placeholder
  } else {
    this.inputRef.current.focus();
  }
}

I.e., there are several variables that change together as the system moves from one state to another. There are invariants that never change:

// assertion that's always true in `componentDidMount`
expect( ( loading && ! inputRef ) || ( ! loading && inputRef ) ).toBeTruthy();

Here, TypeScript nudges us towards refactoring the component to something where the relationship is more clear, e.g., two different components.

I think the PR is okay, I just wanted to kick off a discussion around creating a consistent pattern around modules/imports, but this can be discussed in other issue/PR.

OK, does that mean I can keep the SelectDropdown.Item convention for now? It's an easy codemod if we want to change it later. (we'll need to make the final decision when moving this to @automattic/calypso-ui)

Are the failing checks on the PR a blocker?

Yes, the e2e tests are consistently failing, so I likely broke something. I'll need to look into that.

[diegohaz]

OK, does that mean I can keep the SelectDropdown.Item convention for now? It's an easy codemod if we want to change it later. (we'll need to make the final decision when moving this to @automattic/calypso-ui)

Yes. No need to change that.

[jsnajdr]

Found the bug that was breaking the e2e tests (namely "Revert to Draft" in Classic Editor) and fixed it in c9b3c11.

[jsnajdr]

The remaining e2e failures are not related, let's merge 🤞