Remove Ow #615

clintcs · 2025-01-21T14:17:32Z

🚀 Description

I've been meaning to revisit our use of Ow—especially owSlot() and owSlotType()—for a while now. After doing some research I found the functionality in owSlot() and owSlotType() would be much better as a custom directive.

Modal is the best showcase for how a directive simplifies things. No more firstUpdated() assertions. Fewer refs. And no more "slotchange" listeners with assertions.

Instead, a single directive is placed on the slot we're asserting against:

<li class="action">
  <slot
    name="secondary"
-   @slotchange=${this.#onFooterMenuSecondarySlotChange}
-  ${ref(this.#footerMenuSecondarySlotElementRef)}
+  ${assertSlot([GlideCoreButton], true)}
  ></slot>
</li>

assertSlot() takes two arguments—both optional:

assertSlot(slotted?: (typeof Element | typeof Text)[] | null, isOptional?: boolean)

The first is an array of allowed node types—similar to owSlotType(). The second is a boolean. If a slot is optional, it doesn't have to have content. But, if it does, then the content needs to satisfy the types from the first argument.

Modal's "secondary" slot is an example of a slot that's optional but requires specific content.

Accordion's default slot is an example of a non-optional slot that allows arbitrary content. While assertSlot() simplifies Modal the most, every component will see some simplification—at least a reduction of a few lines:

<slot
  class=${classMap({
    'default-slot': true,
    indented: this.hasPrefixIcon,
  })}
-  @slotchange=${this.#onDefaultSlotChange}
-  ${assertSlot()}
  ${ref(this.#defaultSlotElementRef)}
></slot>

What do you think?

After implementing assertSlot() throughout, Ow became even less useful.

I initially thought Ow was worth using (or at least trying out) to reduce indentation and improve readability. Menu is a case where Ow arguably carried its weight. But most other uses of Ow (aside from it's use via owSlot() and owSlotType()) were marginally useful at best—for example, this use of Ow in Dropdown.

Then there's the fact that we can't replace every conditional with an Ow assertion because some conditionals aren't strictly for type narrowing but exist because the property or element in question may legitimately be undefined.

Plus Ow adds about 9 KB to our package and complicates our Web Test Runner configuration. So it felt like it was time to remove it altogether.

Yay? Nay?

📋 Checklist

I have followed the Contributing Guidelines.
I have added tests to cover new or updated functionality.
I have added or updated Storybook stories.
I have localized new strings.
I have followed the ARIA Authoring Practices Guide or met with the Accessibility Team.
I have included a changeset.
I have scheduled a design review.

🔬 How to Test

📸 Images/Videos of Functionality

N/A

changeset-bot · 2025-01-21T14:17:36Z

🦋 Changeset detected

Latest commit: ecdf00d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@crowdstrike/glide-core	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2025-01-21T16:29:15Z

Preview URL

https://glide-core.crowdstrike-ux.workers.dev/remove-ow

ynotdraw

I like the directive, because it moves the assertions closer to the markup. When a developer is looking at a slot, they can see we assert that slot for certain types of content.

Contrast that with what we have prior to this PR - there's a bit more scrolling around and diving in to understand (checking firstUpdated, among other places).

From what I could tell, there shouldn't be any issue, but I'll ask anyway - any concerns from your end around async slotted content coming in like we had in the past?

ynotdraw · 2025-01-22T00:14:30Z

CONTRIBUTING.md

So much easier/cleaner to document too!

ynotdraw · 2025-01-22T00:15:14Z

src/accordion.test.basics.ts

@@ -2,6 +2,7 @@

 import { expect, fixture, html } from '@open-wc/testing';
 import GlideCoreAccordion from './accordion.js';
+import expectUnhandledRejection from './library/expect-unhandled-rejection.js';


Dig the name of this helper - very clear of its intent 👍

ynotdraw · 2025-01-22T00:16:17Z

src/button-group.test.basics.ts

@@ -3,7 +3,7 @@
 import { expect, fixture, html } from '@open-wc/testing';
 import GlideCoreButtonGroup from './button-group.js';
 import GlideCoreButtonGroupButton from './button-group.button.js';
-import expectArgumentError from './library/expect-argument-error.js';
+import expectWindowError from './library/expect-window-error.js';


This one too!

Hah. Yeah. Figured they'd do better as two separate libraries so the name of each could be exact. They're also general enough to be usable not just for when slots throws.

src/input.ts

ynotdraw · 2025-01-22T00:36:27Z

src/library/assert-slot.test.ts

+it('throws when a typed and required default slot only contains text', async () => {
+  const stub = sinon.stub(console, 'error');
+  const spy = sinon.spy();
+  const onerror = window.onerror;
+
+  // eslint-disable-next-line unicorn/prefer-add-event-listener
+  window.onerror = spy;
+
+  await fixture<GlideCoreWithSlot>(
+    html`<glide-core-with-slot .slotted=${[HTMLButtonElement]}>
+    </glide-core-with-slot>`,
+  );
+
+  expect(spy.callCount).to.equal(1);
+
+  expect(spy.args.at(0)?.at(0)).to.equal(
+    'Uncaught TypeError: Expected GlideCoreWithSlot to have a slotted element that extends HTMLButtonElement.',
+  );
+
+  // eslint-disable-next-line unicorn/prefer-add-event-listener
+  window.onerror = onerror;
+
+  stub.restore();
+});


Reading the test title, I would've expected the default slot to contain text here, but I don't see any text! Oh - unless it's whitespace being added here in the test?

Exactly. I'll throw some text into the slot to make that clearer.

Throw some text into the slot.

✅ f3be546

ynotdraw · 2025-01-22T00:39:40Z

src/library/assert-slot.ts

+    slotted?: (typeof Element | typeof Text)[] | null,
+    isOptional?: boolean,
+  ) {
+    return noChange;


TIL - noChange

Though only useful with custom directives as far as I can tell.

ynotdraw · 2025-01-22T00:41:46Z

src/library/assert-slot.ts

+              ? `Expected the "${part.element.name}" slot of ${host.constructor.name} to have a slotted element that extends ${slotted
+                  .map(({ name }) => name)
+                  .join(' or ')}.`
+              : `Expected ${host.constructor.name} to have a slotted element that extends ${slotted


I was wondering how host.constructor.name's will appear for consumers, but looking at dist/ I think we'll be okay?

Yep. Should appear the same as it does now: GlideCoreCheckbox. We skip mangling class names in Terser for this reason.

Just saw the comment in that file referencing Ow. I'll tweak it before merging.

Tweak comment

✅ f3be546

clintcs · 2025-01-22T11:47:34Z

I like the directive, because it moves the assertions closer to the markup. When a developer is looking at a slot, they can see we assert that slot for certain types of content.

Contrast that with what we have prior to this PR - there's a bit more scrolling around and diving in to understand (checking firstUpdated, among other places).

100%. I would have presented it to the group first for comment. But it struck me as an obvious and significant improvement.

From what I could tell, there shouldn't be any issue, but I'll ask anyway - any concerns from your end around async slotted content coming in like we had in the past?

Great question. No issue. Both Menu Options and Dropdown allow asynchronous slotted content by allowing Text. Though that was the case with Ow as well. Both also have a related test.

As with Ow, these components do assume that consumers are rendering their asynchronous content via a whitespace-producing template—like so:

<glide-core-dropdown>
  ${repeat(
    this.options,
    () => html`<glide-core-dropdown-option></glide-core-dropdown-option>`,
  )}
</glide-core-dropdown>

They make that assumption because we'd otherwise be forced to make the slots optional, and then we could only assert against the type of slotted content and not the existence thereof.

For example, if we allowed a consumer to render asynchronous content programmatically:

import {html, render} from 'lit-html';

const dropdown = document.querySelector('glide-core-dropdown');
const options = html`<glide-core-dropdown-option></glide-core-dropdown-option>;

render(options, dropdown);

<glide-core-dropdown></glide-core-dropdown>

There'd be no slotted whitespace and we'd have to make the slots optional by passing a second argument to assertSlot(). So we'd no longer be able to warn consumers if nothing is slotted.

We'll presumably have to make the slots optional eventually. But, when I initially added slot assertions via Ow, I thought it best to warn consumers of missing slotted content until a use case for programmatic slotted content presented itself.

clintcs · 2025-01-22T14:32:18Z

Pushed an amended commit resolving the merge conflicts from #617.

clintcs · 2025-01-22T17:54:47Z

Pushed another amended commit resolving conflicts from #619.

danwenzel

This is great!

clintcs · 2025-01-22T19:24:42Z

This is great!

Thanks for taking a look!

clintcs force-pushed the remove-ow branch from cc8213d to 34a9974 Compare January 21, 2025 16:24

clintcs force-pushed the remove-ow branch 3 times, most recently from f3cd80b to 3a229f1 Compare January 21, 2025 23:30

clintcs marked this pull request as ready for review January 21, 2025 23:31

clintcs requested review from danwenzel, dylankcrwd and ynotdraw as code owners January 21, 2025 23:31

ynotdraw reviewed Jan 22, 2025

View reviewed changes

ynotdraw approved these changes Jan 22, 2025

View reviewed changes

clintcs force-pushed the remove-ow branch from 0aa058f to 0604efa Compare January 22, 2025 14:32

clintcs force-pushed the remove-ow branch from 0604efa to dd423c8 Compare January 22, 2025 17:56

danwenzel approved these changes Jan 22, 2025

View reviewed changes

Remove Ow

ecdf00d

clintcs force-pushed the remove-ow branch from dd423c8 to ecdf00d Compare January 22, 2025 19:27

clintcs merged commit ca3dc13 into main Jan 22, 2025
7 checks passed

clintcs deleted the remove-ow branch January 22, 2025 19:35

github-actions bot mentioned this pull request Jan 22, 2025

Release Preview #625

Merged

clintcs mentioned this pull request Jan 22, 2025

Add Form Controls Layout support for Radio Group #624

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Ow #615

Remove Ow #615

clintcs commented Jan 21, 2025 •

edited

Loading

changeset-bot bot commented Jan 21, 2025 •

edited

Loading

github-actions bot commented Jan 21, 2025

ynotdraw left a comment

ynotdraw Jan 22, 2025

ynotdraw Jan 22, 2025

ynotdraw Jan 22, 2025

clintcs Jan 22, 2025

ynotdraw Jan 22, 2025

clintcs Jan 22, 2025

clintcs Jan 22, 2025 •

edited

Loading

clintcs Jan 22, 2025

ynotdraw Jan 22, 2025

clintcs Jan 22, 2025

ynotdraw Jan 22, 2025

clintcs Jan 22, 2025

clintcs Jan 22, 2025 •

edited

Loading

clintcs Jan 22, 2025

clintcs commented Jan 22, 2025

clintcs commented Jan 22, 2025

clintcs commented Jan 22, 2025

danwenzel left a comment

clintcs commented Jan 22, 2025

Remove Ow #615

Remove Ow #615

Conversation

clintcs commented Jan 21, 2025 • edited Loading

🚀 Description

📋 Checklist

🔬 How to Test

📸 Images/Videos of Functionality

changeset-bot bot commented Jan 21, 2025 • edited Loading

🦋 Changeset detected

github-actions bot commented Jan 21, 2025

Preview URL

ynotdraw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintcs Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintcs Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintcs commented Jan 22, 2025

clintcs commented Jan 22, 2025

clintcs commented Jan 22, 2025

danwenzel left a comment

Choose a reason for hiding this comment

clintcs commented Jan 22, 2025

clintcs commented Jan 21, 2025 •

edited

Loading

changeset-bot bot commented Jan 21, 2025 •

edited

Loading

clintcs Jan 22, 2025 •

edited

Loading

clintcs Jan 22, 2025 •

edited

Loading