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

Replace class:list implementation with clsx #8142

Merged
merged 4 commits into from
Aug 18, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .changeset/cool-jokes-clap.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
'astro': major
---

Fixes for the `class:list` directive

- Previously, `class:list` would ocassionally not be merged the `class` prop when passed to Astro components. Now, `class:list` is always converted to a `class` prop (as a string value).
- Previously, `class:list` diverged from [`clsx`](https://github.com/lukeed/clsx) in a few edge cases. Now, `class:list` uses [`clsx`](https://github.com/lukeed/clsx) directly.
- `class:list` used to deduplicate matching values, but it no longer does
- `class:list` used to sort individual values, but it no longer does
- `class:list` used to support `Set` and other iterables, but it no longer does
1 change: 1 addition & 0 deletions packages/astro/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@
"boxen": "^6.2.1",
"chokidar": "^3.5.3",
"ci-info": "^3.8.0",
"clsx": "^2.0.0",
"common-ancestor-path": "^1.0.1",
"cookie": "^0.5.0",
"debug": "^4.3.4",
Expand Down
6 changes: 0 additions & 6 deletions packages/astro/src/runtime/server/hydration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import type {
import { AstroError, AstroErrorData } from '../../core/errors/index.js';
import { escapeHTML } from './escape.js';
import { serializeProps } from './serialize.js';
import { serializeListValue } from './util.js';

export interface HydrationMetadata {
directive: string;
Expand Down Expand Up @@ -95,11 +94,6 @@ export function extractDirectives(
break;
}
}
} else if (key === 'class:list') {
if (value) {
// support "class" from an expression passed into a component (#782)
extracted.props[key.slice(0, -5)] = serializeListValue(value);
}
} else {
extracted.props[key] = value;
}
Expand Down
16 changes: 16 additions & 0 deletions packages/astro/src/runtime/server/render/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type {
} from '../../../@types/astro';
import type { RenderInstruction } from './types.js';

import { clsx } from 'clsx';
import { AstroError, AstroErrorData } from '../../../core/errors/index.js';
import { HTMLBytes, markHTMLString } from '../escape.js';
import { extractDirectives, generateHydrateScript } from '../hydration.js';
Expand Down Expand Up @@ -461,6 +462,9 @@ export async function renderComponent(
return await renderFragmentComponent(result, slots);
}

// Ensure directives (`class:list`) are processed
props = normalizeProps(props);

// .html components
if (isHTMLComponent(Component)) {
return await renderHTMLComponent(result, Component, props, slots);
Expand All @@ -473,6 +477,18 @@ export async function renderComponent(
return await renderFrameworkComponent(result, displayName, Component, props, slots);
}

function normalizeProps(props: Record<string, any>): Record<string, any> {
if ('class:list' in props) {
natemoo-re marked this conversation as resolved.
Show resolved Hide resolved
natemoo-re marked this conversation as resolved.
Show resolved Hide resolved
const value = props['class:list'];
delete props['class:list'];
props['class'] = clsx(props['class'], value)
if (props['class'] === '') {
delete props['class']
}
}
return props;
}

export async function renderComponentToString(
result: SSRResult,
displayName: string,
Expand Down
4 changes: 2 additions & 2 deletions packages/astro/src/runtime/server/render/util.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { SSRElement } from '../../../@types/astro';

import { HTMLString, markHTMLString } from '../escape.js';
import { serializeListValue } from '../util.js';
import { clsx } from 'clsx';

export const voidElementNames =
/^(area|base|br|col|command|embed|hr|img|input|keygen|link|meta|param|source|track|wbr)$/i;
Expand Down Expand Up @@ -78,7 +78,7 @@ Make sure to use the static attribute syntax (\`${key}={value}\`) instead of the

// support "class" from an expression passed into an element (#782)
if (key === 'class:list') {
const listValue = toAttributeString(serializeListValue(value), shouldEscape);
const listValue = toAttributeString(clsx(value), shouldEscape);
if (listValue === '') {
return '';
}
Expand Down
30 changes: 0 additions & 30 deletions packages/astro/src/runtime/server/util.ts
Original file line number Diff line number Diff line change
@@ -1,33 +1,3 @@
export function serializeListValue(value: any) {
const hash: Record<string, any> = {};

push(value);

return Object.keys(hash).join(' ');

function push(item: any) {
// push individual iteratables
if (item && typeof item.forEach === 'function') item.forEach(push);
// otherwise, push object value keys by truthiness
else if (item === Object(item))
Object.keys(item).forEach((name) => {
if (item[name]) push(name);
});
// otherwise, push any other values as a string
else {
// get the item as a string
item = item === false || item == null ? '' : String(item).trim();

// add the item if it is filled
if (item) {
item.split(/\s+/).forEach((name: string) => {
hash[name] = true;
});
}
}
}
}

export function isPromise<T = any>(value: any): value is Promise<T> {
return !!value && typeof value === 'object' && typeof value.then === 'function';
}
Expand Down
41 changes: 25 additions & 16 deletions packages/astro/test/astro-class-list.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ describe('Class List', async () => {
const html = await fixture.readFile('/index.html');
const $ = cheerio.load(html);

expect($('[class="test control"]')).to.have.lengthOf(1);
expect($('[class="test expression"]')).to.have.lengthOf(1);
expect($('[class="test true"]')).to.have.lengthOf(1);
expect($('[class="test truthy"]')).to.have.lengthOf(1);
expect($('[class="test set"]')).to.have.lengthOf(1);
expect($('[class="hello goodbye world friend"]')).to.have.lengthOf(1);
expect($('[class="foo baz"]')).to.have.lengthOf(1);
expect($('span:not([class])')).to.have.lengthOf(1);
expect($('[class="test control"]')).to.have.lengthOf(1, '[class="test control"]');
expect($('[class="test expression"]')).to.have.lengthOf(1, '[class="test expression"]');
expect($('[class="test true"]')).to.have.lengthOf(1, '[class="test true"]');
expect($('[class="test truthy"]')).to.have.lengthOf(1, '[class="test truthy"]');
expect($('[class="test set"]')).to.have.lengthOf(1, '[class="test set"]');
expect($('[class="hello goodbye hello world hello friend"]')).to.have.lengthOf(1, '[class="hello goodbye hello world hello friend"]');
expect($('[class="foo baz"]')).to.have.lengthOf(1, '[class="foo baz"]');
expect($('span:not([class])')).to.have.lengthOf(1, 'span:not([class])');

expect($('.false, .noshow1, .noshow2, .noshow3, .noshow4')).to.have.lengthOf(0);
});
Expand All @@ -30,13 +30,22 @@ describe('Class List', async () => {
const html = await fixture.readFile('/component/index.html');
const $ = cheerio.load(html);

expect($('[class="test control"]')).to.have.lengthOf(1);
expect($('[class="test expression"]')).to.have.lengthOf(1);
expect($('[class="test true"]')).to.have.lengthOf(1);
expect($('[class="test truthy"]')).to.have.lengthOf(1);
expect($('[class="test set"]')).to.have.lengthOf(1);
expect($('[class="hello goodbye world friend"]')).to.have.lengthOf(1);
expect($('[class="foo baz"]')).to.have.lengthOf(1);
expect($('span:not([class])')).to.have.lengthOf(1);
expect($('[class="test control"]')).to.have.lengthOf(1, '[class="test control"]');
expect($('[class="test expression"]')).to.have.lengthOf(1, '[class="test expression"]');
expect($('[class="test true"]')).to.have.lengthOf(1, '[class="test true"]');
expect($('[class="test truthy"]')).to.have.lengthOf(1, '[class="test truthy"]');
expect($('[class="test set"]')).to.have.lengthOf(1, '[class="test set"]');
expect($('[class="hello goodbye hello world hello friend"]')).to.have.lengthOf(1, '[class="hello goodbye hello world hello friend"]');
expect($('[class="foo baz"]')).to.have.lengthOf(1, '[class="foo baz"]');
expect($('span:not([class])')).to.have.lengthOf(1, 'span:not([class])');

expect($('[class="test control"]').text()).to.equal('test control');
expect($('[class="test expression"]').text()).to.equal('test expression');
expect($('[class="test true"]').text()).to.equal('test true');
expect($('[class="test truthy"]').text()).to.equal('test truthy');
expect($('[class="test set"]').text()).to.equal('test set');
expect($('[class="hello goodbye hello world hello friend"]').text()).to.equal('hello goodbye hello world hello friend');
expect($('[class="foo baz"]').text()).to.equal('foo baz');
expect($('span:not([class])').text()).to.equal('');
});
});
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<span {...Astro.props} />
<span {...Astro.props}>{Astro.props.class}</span>
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
---
import Component from '../components/Span.astro'
---
<Component class="test control" />

<!-- @note: `class:list` will not be parsed if its value is not an expression -->
<!-- <Component class:list="test" /> -->
<Component class="test control" />

<Component class:list={'test expression'} />

Expand All @@ -13,9 +11,9 @@ import Component from '../components/Span.astro'
<Component class:list={{ test: true, true: true, false: false }} />
<Component class:list={{ test: 1, truthy: '0', noshow1: 0, noshow2: '', noshow3: null, noshow4: undefined }} />

<Component class:list={new Set(['test', 'set'])} />
<Component class:list={['test', 'set']} />

<Component class:list={[ 'hello goodbye', { hello: true, world: true }, new Set([ 'hello', 'friend' ]) ]} />
<Component class:list={[ 'hello goodbye', { hello: true, world: true }, [ 'hello', 'friend' ] ]} />

<Component class:list={['foo', false && 'bar', true && 'baz']} />

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
<span class:list={{ test: true, true: true, false: false }} />
<span class:list={{ test: 1, truthy: '0', noshow1: 0, noshow2: '', noshow3: null, noshow4: undefined }} />

<span class:list={new Set(['test', 'set'])} />
<span class:list={['test', 'set']} />

<span class:list={[ 'hello goodbye', { hello: true, world: true }, new Set([ 'hello', 'friend' ]) ]} />
<span class:list={[ 'hello goodbye', { hello: true, world: true }, [ 'hello', 'friend' ] ]} />

<span class:list={['foo', false && 'bar', true && 'baz']} />

Expand Down
66 changes: 64 additions & 2 deletions packages/astro/test/units/render/components.test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { expect } from 'chai';
import * as cheerio from 'cheerio';
import { fileURLToPath } from 'node:url';
import svelte from '../../../../integrations/svelte/dist/index.js';
import { createFs, createRequestAndResponse, runInContainer } from '../test-utils.js';

const root = new URL('../../fixtures/alias/', import.meta.url);
Expand Down Expand Up @@ -33,7 +32,7 @@ describe('core/render components', () => {
inlineConfig: {
root: fileURLToPath(root),
logLevel: 'silent',
integrations: [svelte()],
integrations: [],
},
},
async (container) => {
Expand All @@ -56,4 +55,67 @@ describe('core/render components', () => {
}
);
});

it('should merge `class` and `class:list`', async () => {
const fs = createFs(
{
'/src/pages/index.astro': `
---
import Class from '../components/Class.astro';
import ClassList from '../components/ClassList.astro';
import BothLiteral from '../components/BothLiteral.astro';
import BothFlipped from '../components/BothFlipped.astro';
import BothSpread from '../components/BothSpread.astro';
---
<Class class="red blue" />
<ClassList class:list={{ red: true, blue: true }} />
<BothLiteral class="red" class:list={{ blue: true }} />
<BothFlipped class:list={{ blue: true }} class="red" />
<BothSpread class:list={{ blue: true }} { ...{ class: "red" }} />
`,
'/src/components/Class.astro': `<pre id="class" set:html={JSON.stringify(Astro.props)} />`,
'/src/components/ClassList.astro': `<pre id="class-list" set:html={JSON.stringify(Astro.props)} />`,
'/src/components/BothLiteral.astro': `<pre id="both-literal" set:html={JSON.stringify(Astro.props)} />`,
'/src/components/BothFlipped.astro': `<pre id="both-flipped" set:html={JSON.stringify(Astro.props)} />`,
'/src/components/BothSpread.astro': `<pre id="both-spread" set:html={JSON.stringify(Astro.props)} />`,
},
root
);

await runInContainer(
{
fs,
inlineConfig: {
root: fileURLToPath(root),
logLevel: 'silent',
integrations: [],
},
},
async (container) => {
const { req, res, done, text } = createRequestAndResponse({
method: 'GET',
url: '/',
});
container.handle(req, res);

await done;
const html = await text();
const $ = cheerio.load(html);

const check = (name) => JSON.parse($(name).text() || '{}')

const Class = check('#class');
const ClassList = check('#class-list');
const BothLiteral = check('#both-literal');
const BothFlipped = check('#both-flipped');
const BothSpread = check('#both-spread');

expect(Class).to.deep.equal({ class: 'red blue' }, '#class');
expect(ClassList).to.deep.equal({ class: 'red blue' }, '#class-list');
expect(BothLiteral).to.deep.equal({ class: 'red blue' }, '#both-literal');
expect(BothFlipped).to.deep.equal({ class: 'red blue' }, '#both-flipped');
expect(BothSpread).to.deep.equal({ class: 'red blue' }, '#both-spread');
}
);
});
});
12 changes: 8 additions & 4 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading