chore: add optional schema prop to plugins

[SethFalco]

Following conversations with maintainers of third-party clients using SVGO, something that has come up is that SVGO does not expose its default behavior or accepted parameters in a way other clients can act on.

This PR introduces a new optional property for plugins called schema, which is a JSON Schema that describes the parameters a plugin accepts and any constraints it may have.

This is intended to imitate Option Schemas in ESLint. Custom Rules in ESLint can expose a schema property, which is a JSON Schema that describes the options the rule supports. Exactly what this PR is adding.

This has numerous uses:

• Third-party clients can now check what parameters are available or validate against them.
• Parameter documentation will live closer to the parameters.
  • svg/svgo.dev will have to move the exported schema property to the plugins respective documentation page.
• We can improve the help pages in CLI by describing the parameters in more detail, including constraints.

This is just a minimal migration/implementation of these types, but there is more to do to clean things up, improve constraints, etc.

To notable things we should also do (doesn't have to be in the JSON Schema):

• Provide human-readable names for plugins. We've decided against this, we should always display plugin names in the way the plugins are referenced in the config to avoid confusion.
• Expose which plugins are enabled by default. (preset-default doesn't do this atm)

Related

• Next major version plan jakearchibald/svgomg#431 (comment)
• Upgrade to svgo v3 sonnyp/OhMySVG#10 (comment)

CC

@jakearchibald

[KTibow]

I would rather schema be generated from TS or TS be generated from schemas than have both in the same codebase.

[SethFalco]

@KTibow See: jakearchibald/svgomg#431 (comment)

[KTibow]

Right, well there are libraries that let you get a typedef rather easily from the JSON schemas introduced in this PR.

FromSchema

The FromSchema method lets you infer TS types directly from JSON schemas:

import { FromSchema } from "json-schema-to-ts";

const dogSchema = {
  type: "object",
  properties: {
    name: { type: "string" },
    age: { type: "integer" },
    hobbies: { type: "array", items: { type: "string" } },
    favoriteFood: { enum: ["pizza", "taco", "fries"] },
  },
  required: ["name", "age"],
} as const;

type Dog = FromSchema<typeof dogSchema>;
// => Will infer the same type as above

[SethFalco]

Interesting, that has a really cool interface. Though, that would also mean shipping a dependency on json-schema-to-ts which is a bit eh.

In general, I would prefer a static solution if we were going to do something like that. Regardless, I've given this one a brief look, and it looks like it only supports TypeScript sources, so that particular library isn't going to work for us anyway

I can get the library to work easily in TypeScript sources, but in JavaScript, nope. I can literally take the same JavaScript file, and tsc will be fine if the plugin ends with .ts but will complain if it ends with .js even though I have the same and valid code in both cases.

[jakearchibald]

Since I'd want to represent this as a slider, what maximum value would you be happy with here?

[SethFalco]

I'm 99% sure we have a limit of 20 somewhere for floatPrecision. However, I wanted some time to peek around in case it varies between plugins.

For now, I'll assume it'll be minimum: 0, maximum: 20, multipleOf: 1. But it's always possible that in this case or others the conclusion is that there shouldn't be a maximum.

The minimum and multipleOf are fixed, it's only maximum which I'm assuming is 20 but need to investigate.

[jakearchibald]

I'll work on the schemas. I guess there'll be some work to make the docs pull from these schemas too?

[jakearchibald]

Is there a particular reason applyTransforms.js doesn't have an exported name?

[KTibow]

Pretty sure it used to be not a real plugin and just imported from convertPathData, so that must've been forgotten in converting it to a plugin

[SethFalco]

@jakearchibald @KTibow

It was not forgotten. The plan was to eventually make it a standalone plugin, and the logic was migrated out of convertPathData until it's considered ready to be used as such.

I don't know what was pending before it's considered ready to be a standalone plugin. However, it is not part of the public API yet.

Reference:

moved into _applyTransforms.js to convert eventually into plugin

— #1428

[jakearchibald]

Cool. I'll use the absence of the name to detect plugins that aren't public.

[SethFalco]

Cool. I'll use the absence of the name to detect plugins that aren't public.

You can use builtin.js to fetch the supported plugins, which exclusively has the ones intended for use from the public API.

import { builtin } from 'svgo/lib/builtin.js';

The only downside to that, is that it includes presets, so you'd want to explicitly ignore preset-default somehow. (We could consider adding a property like preset: true.)

[
  { name: 'preset-default', fn: [Function: fn] },
  {
    name: 'addAttributesToSVGElement',
    description: 'adds attributes to an outer <svg> element',
    fn: [Function (anonymous)]
  },
  {
    name: 'addClassesToSVGElement',
    description: 'adds classnames to an outer <svg> element',
    fn: [Function (anonymous)]
  },
  {
    name: 'cleanupAttrs',
    description: 'cleanups attributes from newlines, trailing and repeating spaces',
    fn: [Function (anonymous)]
  },
  {
    name: 'cleanupEnableBackground',
    description: 'remove or cleanup enable-background attribute when possible',
    fn: [Function (anonymous)]
  },
…

[jakearchibald]

Ohh nice! Is the order of plugins important? preset-default seems to have them in a specific order, whereas builtin has them in alphabetical order.

[SethFalco]

Ohh nice! Is the order of plugins important? preset-default seems to have them in a specific order, whereas builtin has them in alphabetical order.

Shoot… you're right. I was actually thinking about doing the solution I'm about to propose anyway as part of this requirement, so we can resolve that:

Expose which plugins are enabled by default. (preset-default doesn't do this atm)

We can update the createPreset utility in SVGO to export a property containing the array of plugins in the preset in the order they execute in. The createPreset function already receives the array, so we just have to return it too.

For example, the following change, though I'm happy to discuss the interface. We could instead just use a preset property which is immediately assigned the plugin array, rather than have two separate properties.

  export const createPreset = ({ name, plugins }) => {
    return {
      name,
+     isPreset: true,
+     plugins,
      fn: (ast, params, info) => {
        const { floatPrecision, overrides } = params;
        const globalOverrides = {};
        if (floatPrecision != null) {
          globalOverrides.floatPrecision = floatPrecision;
        }
        if (overrides) {
          const pluginNames = plugins.map(({ name }) => name);
          for (const pluginName of Object.keys(overrides)) {
            if (!pluginNames.includes(pluginName)) {
              console.warn(
                `You are trying to configure ${pluginName} which is not part of ${name}.\n` +
                  `Try to put it before or after, for example\n\n` +
                  `plugins: [\n` +
                  `  {\n` +
                  `    name: '${name}',\n` +
                  `  },\n` +
                  `  '${pluginName}'\n` +
                  `]\n`,
              );
            }
          }
        }
        invokePlugins(ast, info, plugins, overrides, globalOverrides);
      },
    };
  };

This way we know which ones are presets explicitly, and you can choose to only get plugins from a particular preset, including order.

[SethFalco]

@jakearchibald

Sorry, just noticed I missed a question.

I guess there'll be some work to make the docs pull from these schemas too?

Yeah, there will be. But no worries, I can address this. We'll probably opt to handle that in svgo.dev. We can pull the schema out, JSON.stringify it, and inject it into the front matter of the respective documentation page during build. (Perks to YAML being a superset of JSON.)

[jakearchibald]

Currently I'm exporting the plugins from preset-default: https://github.com/jakearchibald/svgo/blob/chore/schema/plugins/preset-default.js#L39-L76

Then over in SVGOMG:

import { plugins as defaultPlugins } from 'svgo/plugins/preset-default';
import presetDefault from 'svgo/plugins/preset-default';
import { builtin } from 'svgo/lib/builtin';

const builtinSet = new Set(builtin);
builtinSet.delete(presetDefault);

const allPlugins = new Set([
  // Get the initial order from preset-default
  ...defaultPlugins,
  // Add the rest in whatever order
  ...builtinSet,
]);

I'm happy to keep doing this, or the preset/plugins thing.

[SethFalco]

Before continuing, just want to say thank you for taking this on!

---

Currently I'm exporting the plugins from preset-default: https://github.com/jakearchibald/svgo/blob/chore/schema/plugins/preset-default.js#L39-L76

I would prefer if we took the approach I proposed (which would achieve a similar interface) as it's easier to maintain on our side in the case that we make more presets, which is an idea that's been thrown around a few times.

With the way you did it, while it does mean the list is defined once which is great, it's simpler if presets didn't have to know to export another property imo.

Then you'd do:

import presetDefault from 'svgo/plugins/preset-default';
import { builtin } from 'svgo/lib/builtin';

const builtinSet = new Set(builtin);
builtinSet.delete(presetDefault);

const allPlugins = new Set([
  // Get the initial order from preset-default
  ...presetDefault.plugins,
  // Add the rest in whatever order
  ...builtinSet,
]);

[jakearchibald]

No problem, I'll do that.

[jakearchibald]

I'm going to add a title export from each plugin, which will be the same as the title used in the mdx pages.

For description, how do you feel about:

export const description = "Adds attributes to the outer most [`<svg>`](https://developer.mozilla.org/docs/Web/SVG/Element/svg) element in the document. This is not an optimization and will increase the size of SVG documents.";

As in, supporting markdown here. I don't mind either way.

Edit: I see markdown is used in the description schema fields, so I assume this is ok?

[jakearchibald]

A lot of this description only applies to the JS API, and wouldn't apply to the UI. I'm not sure what to do about that, because the docs need the JS details, but they don't make sense in SVGOMG.

[SethFalco]

@jakearchibald

I'm going to add a title export from each plugin, which will be the same as the title used in the mdx pages.

I'm not into the idea tbh. I'm happy for SVGO to make itself easier for frontend applications to wrap around, but I don't want to expose that much of a public interface that we'd be exporting the same property multiple times for different clients.

I'd like for our plugins to keep JS friendly plugin IDs, then you could either map them on your end, or use a function to create a UI friendly name from the plugin ID.

It's not the perfect solution, but I think it's better than introducing things that don't serve SVGO's needs into our public API. A frontend application's interface is very flexible, but as a library once we introduce new properties, we're pretty much stuck with it for a while, and exporting a name and title separately isn't something I want stuck in SVGO for now.

I was also hesitant to add UI-friendly names for individual parameters when I started working on this, but I opted to run with it since the parameter is meant to be a JSON Schema and that's part of the spec.

Note: Even on the SVGO documentation we don't clean up param names, and I'm actually thinking we'll drop the custom title field on the docs so it's consistent with the plugin ID.

For description, how do you feel about:

In general, I'm fine with this. However, let's save that for when SVGO is prepared to do it's v4 release. That change drastically changes the structure, length, and markup of the property, which is part of our public interface.

A lot of this description only applies to the JS API, and wouldn't apply to the UI. I'm not sure what to do about that, because the docs need the JS details, but they don't make sense in SVGOMG.

You'd probably want to override specific strings on your side for when something we do doesn't make sense for you.

That way, you can use our docs when it's available, but you can override it when your adaptation/wrapper doesn't match our interface 1-2-1.

We definitely can't handle this on our side, as SVGOMG is just one example of a third-party client, and we can't maintain docs here that are specific to your client when other clients might represent this differently.

Probably something like the t() (name of the function most i18n libraries export) that lets you swap out, normalize, or localize strings that SVGO give you.

Edit:

I realize I just contradicted this:

Provide human-readable names for plugins.

In theory, I was all for it, but now that we're actually implementing, I have some reservations with that way of approaching it. We could explore this topic more in v4. For now, I'd prefer if we just focused on the following changes:

• Improve documentation accessible from within the JS API, without making significant changes to existing properties.
• Export the JSON Schema for plugin parameters, including constraints.
• It should be possible to find out which plugins a preset include.

Basically, everything we discussed and more, except human-readable names. 🤔

[jakearchibald]

I'm going to add a title export from each plugin, which will be the same as the title used in the mdx pages.

I'm not into the idea tbh. I'm happy for SVGO to make itself easier for frontend applications to wrap around, but I don't want to expose that much of a public interface that we'd be exporting the same property multiple times for different clients

My intent was that the docs would use this exported name too, rather than duplicating it.

[SethFalco]

My intent was that the docs would use this exported name too, rather than duplicating it.

When I said duplicating, I meant the name vs title, two properties that mean the same thing but aimed at different interfaces.

I don't want to clutter the JavaScript interface with these details. That should be managed outside of it, imo. For the docs, we'll either use the exported names, or map them to human-readable names in svgo.dev, but not pollute the JavaScript interface with properties exclusively for documentation/clients. That's better managed externally.