From 1b3c7de8d3b1789ca8d471a0d75241d356a80097 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Fri, 6 Dec 2024 17:27:07 -0800 Subject: [PATCH] Represent rest parameters as properties on Parameter (#2454) This simplifies the code and will likely make it easier for users to consume and manipulate parameters. --- .../__snapshots__/parameter-list.test.ts.snap | 2 +- .../src/__snapshots__/parameter.test.ts.snap | 12 ++ .../lib/src/parameter-list.test.ts | 173 +----------------- pkg/sass-parser/lib/src/parameter-list.ts | 54 +----- pkg/sass-parser/lib/src/parameter.test.ts | 134 +++++++++++++- pkg/sass-parser/lib/src/parameter.ts | 65 +++++-- 6 files changed, 204 insertions(+), 236 deletions(-) diff --git a/pkg/sass-parser/lib/src/__snapshots__/parameter-list.test.ts.snap b/pkg/sass-parser/lib/src/__snapshots__/parameter-list.test.ts.snap index 2c93129bc..96fc0f068 100644 --- a/pkg/sass-parser/lib/src/__snapshots__/parameter-list.test.ts.snap +++ b/pkg/sass-parser/lib/src/__snapshots__/parameter-list.test.ts.snap @@ -11,9 +11,9 @@ exports[`a parameter list toJSON 1`] = ` ], "nodes": [ <$foo>, + <$bar...>, ], "raws": {}, - "restParameter": "bar", "sassType": "parameter-list", "source": <1:12-1:27 in 0>, } diff --git a/pkg/sass-parser/lib/src/__snapshots__/parameter.test.ts.snap b/pkg/sass-parser/lib/src/__snapshots__/parameter.test.ts.snap index 5889a1b3e..56d9b6793 100644 --- a/pkg/sass-parser/lib/src/__snapshots__/parameter.test.ts.snap +++ b/pkg/sass-parser/lib/src/__snapshots__/parameter.test.ts.snap @@ -12,6 +12,7 @@ exports[`a parameter toJSON with a default 1`] = ` ], "name": "baz", "raws": {}, + "rest": false, "sassType": "parameter", "source": <1:13-1:24 in 0>, } @@ -28,7 +29,18 @@ exports[`a parameter toJSON with no default 1`] = ` ], "name": "baz", "raws": {}, + "rest": false, "sassType": "parameter", "source": <1:13-1:17 in 0>, } `; + +exports[`a parameter toJSON with rest = true 1`] = ` +{ + "inputs": [], + "name": "baz", + "raws": {}, + "rest": true, + "sassType": "parameter", +} +`; diff --git a/pkg/sass-parser/lib/src/parameter-list.test.ts b/pkg/sass-parser/lib/src/parameter-list.test.ts index 2c2b5aa11..6cbf1617e 100644 --- a/pkg/sass-parser/lib/src/parameter-list.test.ts +++ b/pkg/sass-parser/lib/src/parameter-list.test.ts @@ -20,9 +20,6 @@ describe('a parameter list', () => { expect(node.sassType).toBe('parameter-list')); it('has no nodes', () => expect(node.nodes).toHaveLength(0)); - - it('has no rest parameter', () => - expect(node.restParameter).toBeUndefined()); }); } @@ -80,9 +77,6 @@ describe('a parameter list', () => { expect(node.nodes[0].defaultValue).toBeUndefined(); expect(node.nodes[0].parent).toBe(node); }); - - it('has no rest parameter', () => - expect(node.restParameter).toBeUndefined()); }); } @@ -165,9 +159,6 @@ describe('a parameter list', () => { expect(node.nodes[0]).toHaveStringExpression('defaultValue', 'bar'); expect(node.nodes[0]).toHaveProperty('parent', node); }); - - it('has no rest parameter', () => - expect(node.restParameter).toBeUndefined()); }); } @@ -253,58 +244,6 @@ describe('a parameter list', () => { }); }); - describe('with a rest parameter', () => { - function describeNode( - description: string, - create: () => ParameterList, - ): void { - describe(description, () => { - beforeEach(() => void (node = create())); - - it('has a sassType', () => - expect(node.sassType).toBe('parameter-list')); - - it('has no nodes', () => expect(node.nodes).toHaveLength(0)); - - it('has a rest parameter', () => - expect(node.restParameter).toBe('foo')); - }); - } - - describeNode( - 'parsed as SCSS', - () => - (scss.parse('@function x($foo...) {}').nodes[0] as FunctionRule) - .parameters, - ); - - describeNode( - 'parsed as Sass', - () => - (sass.parse('@function x($foo...)').nodes[0] as FunctionRule) - .parameters, - ); - - describeNode( - 'constructed manually', - () => new ParameterList({restParameter: 'foo'}), - ); - - describeNode( - 'constructed from properties', - () => - new FunctionRule({ - functionName: 'x', - parameters: {restParameter: 'foo'}, - }).parameters, - ); - }); - - it('assigned a new rest parameter', () => { - node.restParameter = 'qux'; - expect(node.restParameter).toBe('qux'); - }); - describe('can add', () => { beforeEach(() => void (node = new ParameterList())); @@ -683,17 +622,10 @@ describe('a parameter list', () => { }); describe('stringifies', () => { - describe('with no nodes or rest parameter', () => { + describe('with no nodes', () => { it('with default raws', () => expect(new ParameterList().toString()).toBe('()')); - it('ignores restParameter', () => - expect( - new ParameterList({ - raws: {restParameter: {value: 'foo', raw: 'foo'}}, - }).toString(), - ).toBe('()')); - it('ignores comma', () => expect(new ParameterList({raws: {comma: true}}).toString()).toBe('()')); @@ -709,22 +641,6 @@ describe('a parameter list', () => { '($foo, $bar, $baz)', )); - it('ignores beforeRestParameter', () => - expect( - new ParameterList({ - nodes: ['foo', 'bar', 'baz'], - raws: {beforeRestParameter: '/**/'}, - }).toString(), - ).toBe('($foo, $bar, $baz)')); - - it('ignores restParameter', () => - expect( - new ParameterList({ - nodes: ['foo', 'bar', 'baz'], - raws: {restParameter: {value: 'foo', raw: 'foo'}}, - }).toString(), - ).toBe('($foo, $bar, $baz)')); - it('with comma: true', () => expect( new ParameterList({ @@ -788,77 +704,6 @@ describe('a parameter list', () => { ).toBe('($foo, $bar, $baz ,)')); }); }); - - describe('with restParameter', () => { - it('with default raws', () => - expect(new ParameterList({restParameter: 'foo'}).toString()).toBe( - '($foo...)', - )); - - it("that's not an identifier", () => - expect(new ParameterList({restParameter: 'f o'}).toString()).toBe( - '($f\\20o...)', - )); - - it('with parameters', () => - expect( - new ParameterList({ - nodes: ['foo', 'bar'], - restParameter: 'baz', - }).toString(), - ).toBe('($foo, $bar, $baz...)')); - - describe('with beforeRestParameter', () => { - it('with no parameters', () => - expect( - new ParameterList({ - restParameter: 'foo', - raws: {beforeRestParameter: '/**/'}, - }).toString(), - ).toBe('(/**/$foo...)')); - - it('with parameters', () => - expect( - new ParameterList({ - nodes: ['foo', 'bar'], - restParameter: 'baz', - raws: {beforeRestParameter: '/**/'}, - }).toString(), - ).toBe('($foo, $bar,/**/$baz...)')); - }); - - it('with matching restParameter', () => - expect( - new ParameterList({ - restParameter: 'foo', - raws: {restParameter: {value: 'foo', raw: 'f\\6fo'}}, - }).toString(), - ).toBe('($f\\6fo...)')); - - it('with non-matching restParameter', () => - expect( - new ParameterList({ - restParameter: 'foo', - raws: {restParameter: {value: 'bar', raw: 'b\\61r'}}, - }).toString(), - ).toBe('($foo...)')); - - it('ignores comma', () => - expect( - new ParameterList({ - restParameter: 'foo', - raws: {comma: true}, - }).toString(), - ).toBe('($foo...)')); - - it('with after', () => - expect( - new ParameterList({ - restParameter: 'foo', - raws: {after: '/**/'}, - }).toString(), - ).toBe('($foo.../**/)')); - }); }); describe('clone', () => { @@ -867,7 +712,6 @@ describe('a parameter list', () => { () => void (original = new ParameterList({ nodes: ['foo', 'bar'], - restParameter: 'baz', raws: {after: ' '}, })), ); @@ -882,11 +726,8 @@ describe('a parameter list', () => { expect(clone.nodes[0].parent).toBe(clone); expect(clone.nodes[1].name).toBe('bar'); expect(clone.nodes[1].parent).toBe(clone); - expect(clone.restParameter).toBe('baz'); }); - it('restParameter', () => expect(clone.restParameter).toBe('baz')); - it('raws', () => expect(clone.raws).toEqual({after: ' '})); it('source', () => expect(clone.source).toBe(original.source)); @@ -932,18 +773,6 @@ describe('a parameter list', () => { expect(clone.nodes[1].name).toBe('bar'); }); }); - - describe('restParameter', () => { - it('defined', () => - expect(original.clone({restParameter: 'qux'}).restParameter).toBe( - 'qux', - )); - - it('undefined', () => - expect( - original.clone({restParameter: undefined}).restParameter, - ).toBeUndefined()); - }); }); }); diff --git a/pkg/sass-parser/lib/src/parameter-list.ts b/pkg/sass-parser/lib/src/parameter-list.ts index e82f314f5..bf9561adc 100644 --- a/pkg/sass-parser/lib/src/parameter-list.ts +++ b/pkg/sass-parser/lib/src/parameter-list.ts @@ -8,7 +8,6 @@ import {Container} from './container'; import {Parameter, ParameterProps} from './parameter'; import {LazySource} from './lazy-source'; import {Node} from './node'; -import {RawWithValue} from './raw-with-value'; import * as sassInternal from './sass-internal'; import * as utils from './utils'; @@ -32,7 +31,6 @@ export type NewParameters = */ export interface ParameterListObjectProps { nodes?: ReadonlyArray; - restParameter?: string; raws?: ParameterListRaws; } @@ -51,22 +49,10 @@ export type ParameterListProps = * @category Statement */ export interface ParameterListRaws { - /** Whitespace before the rest parameter, if one exists. */ - beforeRestParameter?: string; - - /** - * The name of the rest parameter, if one exists. - * - * This may be different than {@link ParameterList.restParameter} if the name - * contains escape codes or underscores. - */ - restParameter?: RawWithValue; - /** * Whether the final parameter has a trailing comma. * - * Ignored if {@link ParameterList.nodes} is empty or if - * {@link ParameterList.restParameter} is set. + * Ignored if {@link ParameterList.nodes} is empty. */ comma?: boolean; @@ -99,15 +85,6 @@ export class ParameterList } private declare _nodes?: Array; - /** - * The name of the rest parameter (such as `args` in `...$args`) in this - * parameter list. - * - * This is the parsed and normalized value, with underscores converted to - * hyphens and escapes resolved to the characters they represent. - */ - declare restParameter?: string; - /** * Iterators that are currently active within this parameter list. Their * indices refer to the last position that has already been sent to the @@ -124,27 +101,26 @@ export class ParameterList this.source = new LazySource(inner); // TODO: set lazy raws here to use when stringifying this._nodes = []; - this.restParameter = inner.restArgument ?? undefined; for (const argument of inner.arguments) { this.append(new Parameter(undefined, argument)); } + if (inner.restArgument) { + // TODO: Provide source information for this argument. + this.append({name: inner.restArgument, rest: true}); + } } if (this._nodes === undefined) this._nodes = []; } clone(overrides?: Partial): this { - return utils.cloneNode(this, overrides, [ - 'nodes', - {name: 'restParameter', explicitUndefined: true}, - 'raws', - ]); + return utils.cloneNode(this, overrides, ['nodes', 'raws']); } toJSON(): object; /** @hidden */ toJSON(_: string, inputs: Map): object; toJSON(_?: string, inputs?: Map): object { - return utils.toJSON(this, ['nodes', 'restParameter'], inputs); + return utils.toJSON(this, ['nodes'], inputs); } append(...nodes: NewParameters[]): this { @@ -283,21 +259,7 @@ export class ParameterList result += parameter.toString(); result += parameter.raws.after ?? ''; } - - if (this.restParameter) { - if (this.nodes.length) { - result += ',' + (this.raws.beforeRestParameter ?? ' '); - } else if (this.raws.beforeRestParameter) { - result += this.raws.beforeRestParameter; - } - result += - '$' + - (this.raws.restParameter?.value === this.restParameter - ? this.raws.restParameter.raw - : sassInternal.toCssIdentifier(this.restParameter)) + - '...'; - } - if (this.raws.comma && this.nodes.length && !this.restParameter) { + if (this.raws.comma && this.nodes.length) { result += ','; } return result + (this.raws.after ?? '') + ')'; diff --git a/pkg/sass-parser/lib/src/parameter.test.ts b/pkg/sass-parser/lib/src/parameter.test.ts index 336d67463..5d6df3a09 100644 --- a/pkg/sass-parser/lib/src/parameter.test.ts +++ b/pkg/sass-parser/lib/src/parameter.test.ts @@ -33,6 +33,8 @@ describe('a parameter', () => { it('has no default value', () => expect(node.defaultValue).toBeUndefined()); + + it('is not a rest parameter', () => expect(node.rest).toBe(false)); }); } @@ -81,6 +83,8 @@ describe('a parameter', () => { it('has a default value', () => expect(node).toHaveStringExpression('defaultValue', 'bar')); + + it('is not a rest parameter', () => expect(node.rest).toBe(false)); }); } @@ -218,16 +222,89 @@ describe('a parameter', () => { }); }); + describe('as a rest parameter', () => { + function describeNode(description: string, create: () => Parameter): void { + describe(description, () => { + beforeEach(() => (node = create())); + + it('has a sassType', () => + expect(node.sassType.toString()).toBe('parameter')); + + it('has a name', () => expect(node.name).toBe('foo')); + + it('has no default value', () => + expect(node.defaultValue).toBeUndefined()); + + it('is a rest parameter', () => expect(node.rest).toBe(true)); + }); + } + + describeNode( + 'parsed as SCSS', + () => + (scss.parse('@function a($foo...) {}').nodes[0] as FunctionRule) + .parameters.nodes[0], + ); + + describeNode( + 'parsed as Sass', + () => + (sass.parse('@function a($foo...)').nodes[0] as FunctionRule).parameters + .nodes[0], + ); + + describeNode( + 'constructed manually', + () => new Parameter({name: 'foo', rest: true}), + ); + + describeNode( + 'constructed from properties', + () => new ParameterList({nodes: [{name: 'foo', rest: true}]}).nodes[0], + ); + }); + it('assigned a new name', () => { node.name = 'baz'; expect(node.name).toBe('baz'); }); - it('assigned a new default', () => { - const old = node.defaultValue!; - node.defaultValue = {text: 'baz', quotes: true}; - expect(old.parent).toBeUndefined(); - expect(node).toHaveStringExpression('defaultValue', 'baz'); + describe('assigned a new default', () => { + it('updates the default', () => { + const old = node.defaultValue!; + node.defaultValue = {text: 'baz'}; + expect(old.parent).toBeUndefined(); + expect(node).toHaveStringExpression('defaultValue', 'baz'); + }); + + it('sets rest to false', () => { + node.rest = true; + node.defaultValue = {text: 'baz'}; + expect(node.rest).toBe(false); + }); + + it('leaves rest alone if defaultValue is undefined', () => { + node.rest = true; + node.defaultValue = undefined; + expect(node.rest).toBe(true); + }); + }); + + describe('assigned rest = true', () => { + it('updates the value of rest', () => { + node.rest = true; + expect(node.rest).toBe(true); + }); + + it('sets defaultValue to undefined', () => { + node.rest = true; + expect(node.defaultValue).toBe(undefined); + }); + + it('leaves defaultValue alone if rest is false', () => { + node.rest = false; + expect(node).toHaveStringExpression('defaultValue', 'bar'); + }); }); describe('stringifies', () => { @@ -241,6 +318,11 @@ describe('a parameter', () => { new Parameter(['foo', {text: 'bar', quotes: true}]).toString(), ).toBe('$foo: "bar"')); + it('with rest', () => + expect(new Parameter(['foo', {rest: true}]).toString()).toBe( + '$foo...', + )); + it('with a non-identifier name', () => expect(new Parameter('f o').toString()).toBe('$f\\20o')); }); @@ -287,6 +369,23 @@ describe('a parameter', () => { }).toString(), ).toBe('$foo')); + it('with beforeRest', () => + expect( + new Parameter({ + name: 'foo', + rest: true, + raws: {beforeRest: '/**/'}, + }).toString(), + ).toBe('$foo/**/...')); + + it('ignores beforeRest with rest = false', () => + expect( + new Parameter({ + name: 'foo', + raws: {beforeRest: '/**/'}, + }).toString(), + ).toBe('$foo')); + // raws.before is only used as part of a Configuration describe('ignores after', () => { it('with no default', () => @@ -305,6 +404,15 @@ describe('a parameter', () => { raws: {after: '/**/'}, }).toString(), ).toBe('$foo: "bar"')); + + it('with rest = true', () => + expect( + new Parameter({ + name: 'foo', + rest: true, + raws: {after: '/**/'}, + }).toString(), + ).toBe('$foo...')); }); }); }); @@ -327,6 +435,8 @@ describe('a parameter', () => { it('defaultValue', () => expect(clone).toHaveStringExpression('defaultValue', 'bar')); + + it('rest', () => expect(clone.rest).toBe(false)); }); describe('creates a new', () => { @@ -370,6 +480,14 @@ describe('a parameter', () => { original.clone({defaultValue: undefined}).defaultValue, ).toBeUndefined()); }); + + describe('rest', () => { + it('defined', () => + expect(original.clone({rest: true}).rest).toBe(true)); + + it('undefined', () => + expect(original.clone({rest: undefined}).rest).toBe(false)); + }); }); }); @@ -385,5 +503,11 @@ describe('a parameter', () => { (scss.parse('@function x($baz) {}').nodes[0] as FunctionRule).parameters .nodes[0], ).toMatchSnapshot()); + + it('with rest = true', () => + expect( + (scss.parse('@function x($baz...) {}').nodes[0] as FunctionRule) + .parameters.nodes[0], + ).toMatchSnapshot()); }); }); diff --git a/pkg/sass-parser/lib/src/parameter.ts b/pkg/sass-parser/lib/src/parameter.ts index ea5acdbd7..a3a869027 100644 --- a/pkg/sass-parser/lib/src/parameter.ts +++ b/pkg/sass-parser/lib/src/parameter.ts @@ -32,11 +32,17 @@ export interface ParameterRaws { name?: RawWithValue; /** - * The whitespace and colon between the parameter name and default value. This - * is ignored unless the parameter has a default value. + * The whitespace and colon between the parameter name and default value, if + * it has one. */ between?: string; + /** + * The whitespace between the parameter name and the `...`, if {@link + * Parameter.rest} is true. + */ + beforeRest?: string; + /** * The space symbols between the end of the parameter (after the default value * if it has one or the parameter name if it doesn't) and the comma afterwards. @@ -51,11 +57,19 @@ export interface ParameterRaws { * * @category Statement */ -export interface ParameterObjectProps { +export type ParameterObjectProps = { raws?: ParameterRaws; name: string; - defaultValue?: Expression | ExpressionProps; -} +} & ( + | { + defaultValue?: Expression | ExpressionProps; + rest?: never; + } + | { + defaultValue?: never; + rest?: boolean; + } +); /** * Properties used to initialize a {@link Parameter} without an explicit name. @@ -97,7 +111,11 @@ export class Parameter extends Node { */ declare name: string; - /** The expression that provides the default value for the parameter. */ + /** + * The expression that provides the default value for the parameter. + * + * Setting this to a value automatically sets {@link rest} to `false`. + */ get defaultValue(): Expression | undefined { return this._defaultValue!; } @@ -106,6 +124,7 @@ export class Parameter extends Node { if (!value) { this._defaultValue = undefined; } else { + this._rest = false; if (!('sassType' in value)) value = fromProps(value); if (value) value.parent = this; this._defaultValue = value; @@ -113,6 +132,21 @@ export class Parameter extends Node { } private declare _defaultValue?: Expression; + /** + * Whether this is a rest parameter (indicated by `...` in Sass). + * + * Setting this to true automatically sets {@link defaultValue} to + * `undefined`. + */ + get rest(): boolean { + return this._rest ?? false; + } + set rest(value: boolean) { + if (value) this.defaultValue = undefined; + this._rest = value; + } + private declare _rest?: boolean; + constructor(defaults: ParameterProps); /** @hidden */ constructor(_: undefined, inner: sassInternal.Argument); @@ -120,14 +154,17 @@ export class Parameter extends Node { if (typeof defaults === 'string') { defaults = {name: defaults}; } else if (Array.isArray(defaults)) { - const [name, rest] = defaults; - if ('sassType' in rest || !('defaultValue' in rest)) { + const [name, props] = defaults; + if ( + 'sassType' in props || + !('defaultValue' in props || 'rest' in props) + ) { defaults = { name, - defaultValue: rest as Expression | ExpressionProps, + defaultValue: props as Expression | ExpressionProps, }; } else { - defaults = {name, ...rest}; + defaults = {name, ...props} as ParameterObjectProps; } } super(defaults); @@ -147,6 +184,7 @@ export class Parameter extends Node { 'raws', 'name', {name: 'defaultValue', explicitUndefined: true}, + 'rest', ]); } @@ -154,7 +192,7 @@ export class Parameter extends Node { /** @hidden */ toJSON(_: string, inputs: Map): object; toJSON(_?: string, inputs?: Map): object { - return utils.toJSON(this, ['name', 'defaultValue'], inputs); + return utils.toJSON(this, ['name', 'defaultValue', 'rest'], inputs); } /** @hidden */ @@ -164,7 +202,10 @@ export class Parameter extends Node { (this.raws.name?.value === this.name ? this.raws.name.raw : sassInternal.toCssIdentifier(this.name)) + - (this.defaultValue ? (this.raws.between ?? ': ') + this.defaultValue : '') + (this.defaultValue + ? (this.raws.between ?? ': ') + this.defaultValue + : '') + + (this.rest ? (this.raws.beforeRest ?? '') + '...' : '') ); }