Consider a simpler API

[Rich-Harris]

It feels like we can make the API simpler. Why isn't the community template just this, for example?

import { defineIntegration } from '@sveltejs/cli/integration';

export default defineIntegration({
  description: 'An adder template demo',
  environments: { kit: true, svelte: true },
  options: {
    demo: {
      question: 'Do you want to use a demo?',
      type: 'boolean',
      default: false
    }
  },
  files: ({ options }) => [
    {
      name: 'adder-template-demo.txt',
      content: (file) => {
        if (options.demo) {
          return 'This is a text file made by the integration template demo!'
        }

        return file.content;
      }
    },
    {
      name: 'src/DemoComponent.svelte',
      content: (file) => {
        file.addImport('../adder-template-demo.txt?raw', 'default', 'Demo');
      }
    {
  ]
});

Things to note:

• it's a single file. To me this is the difference between 'oh hey I can build one of these!' and 'ugh. strap in, I guess'. It probably doesn't even need a src directory
• id and name have been removed. They don't need to be separate concepts — it's less confusing if we just use an id consistently, instead of leaving people who see 'Foo Bar' listed somewhere to wonder whether it's foobar or foo-bar or whatever. And for stuff on npm, the package name is the id, I'd argue we don't need another (that could potentially be something different than the package name, which is another source of confusion). Maybe there's an argument for mandating an id for programmatic usage but I think it's probably unnecessary
• description and environments moved up to the top level instead of being in metadata
• no packages array. Surely package.json can be just like any other JSON file? The smaller the API surface area the better
• files is a function that returns an array, instead of an array of objects. That way it's easier to put logic in one place (e.g. you can do const ext = options.typescript ? 'ts' : 'js' once instead of in each name method, and easier to return a different set of files entirely under certain conditions
• contentType is inferred from file extension (I don't know if this is possible to do reliably from a types perspective, just an idea)
• utilities like addImport are passed in rather than imported. The big downside here is that it slightly discourages you from extracting the logic out into separate functions, but I'd argue a) this is a context where less indirection is better anyway, and b) it's outweighed by the benefits of discoverability and not having to deal with the exposed guts of things like jsAst unless you absolutely have to

Related note: What's the technical reason for the prohibition on dependencies? Seems very restrictive

[benmccann]

Yeah. I agree things can be simpler. Sent #81 and #83 to start on that.

contentType is inferred from file extension (I don't know if this is possible to do reliably from a types perspective, just an idea)

Going away with #75

files is a function that returns an array, instead of an array of objects. That way it's easier to put logic in one place (e.g. you can do const ext = options.typescript ? 'ts' : 'js' once instead of in each name method, and easier to return a different set of files entirely under certain conditions

@manuel3108 @AdrianGonz97 you may be interested in this idea as I just saw you discussing something like #67 (comment)

Related note: What's the technical reason for the prohibition on dependencies? Seems very restrictive

Something to do with how community adders are executed. The thing that has been taking my attention is that we have a very big API surface area. I've been putting together some PRs today trying to shrink it down, but even still I'm rather nervous that we'll want to change something in the future. I'd like to see if we can't make community adders more standalone (#77)

[AdrianGonz97]

• files is a function that returns an array, instead of an array of objects. That way it's easier to put logic in one place (e.g. you can do const ext = options.typescript ? 'ts' : 'js' once instead of in each name method, and easier to return a different set of files entirely under certain conditions

funnily enough, this would solve my qualm with #67 (comment)

• no packages array. Surely package.json can be just like any other JSON file? The smaller the API surface area the better

we need to pass conditionals so that certain packages are installed only under certain conditions. for instance, see the tailwind adder integration:

cli/packages/adders/tailwindcss/config/adder.ts

Lines 21 to 35 in 59ddada

	packages: [
	{ name: 'tailwindcss', version: '^3.4.9', dev: true },
	{ name: 'autoprefixer', version: '^10.4.20', dev: true },
	{
	name: '@tailwindcss/typography',
	version: '^0.5.14',
	dev: true,
	condition: ({ options }) => options.plugins.includes('typography')
	},
	{
	name: 'prettier-plugin-tailwindcss',
	version: '^0.6.5',
	dev: true,
	condition: ({ prettier }) => prettier
	}

• contentType is inferred from file extension (I don't know if this is possible to do reliably from a types perspective, just an idea)

this is being worked on in a different way. see #33 and #75

edit: oh, ben answered too. i guess github decided to not live update the comments today 🙃

[AdrianGonz97]

Related note: What's the technical reason for the prohibition on dependencies? Seems very restrictive

since we're downloading and unpacking the community packages manually, it'll require us to resolve all of their dependencies as well. this is a solvable problem. it's just not a priority atm and I expect it to be a pain in the ass to do when the time comes 😄

for now, community packages can just bundle their deps if they desperately need them

[Rich-Harris]

we need to pass conditionals so that certain packages are installed only under certain conditions

But that would be part of the context passed to the files function, no?

Another option could be something along these lines:

export default defineIntegration({
  description: 'An adder template demo',
  environments: { kit: true, svelte: true },
  options: {
    demo: {
      question: 'Do you want to use a demo?',
      type: 'boolean',
      default: false
    }
  },
  setup: ({ api, options }) => {
    if (options.demo) {
      api.createFile('adder-template-demo.txt', 'This is a text file made by the integration template demo!', {
        force: true // if false or unspecified, errors if file exists
      });
    }

    api.updateFileIfExists('src/DemoComponent.svelte', (file) => {
      const { script, css, generateCode } = parseSvelte(file.content);
      file.addImport('../adder-template-demo.txt?raw', 'default', 'Demo');

      imports.addDefault(script.ast, '../adder-template-demo.txt?raw', 'default', 'Demo');
      return generateCode({...});
    });
  }
});

The details of the APIs aren't important here (related: I find the generateCode stuff slightly cumbersome if I'm honest), the general thought is that authors have a lot more flexibility with this approach than if they're hemmed in by whatever declarative affordances we thought to provide, and it ends up being less code that's easier to debug.

[Rich-Harris]

(For that matter I'm not sure we would need things like api.createFile — people can just use fs)

[AdrianGonz97]

But that would be part of the context passed to the files function, no?

yes, but are you suggesting that adders should be the ones that update the package.json directly to install the packages? if so then that feels messy, especially when it's going to be a super common action

Another option could be something along these lines:

I much prefer what you originally proposed - that's getting closer to my ultimate vision of having a tsmorph-like api that performs string manipulations behind the scenes (so we don't have to faff around with generating code from ASTs, which is the leading cause of all of our formatting woes).

something like this would be ideal:

import { defineIntegration } from '@sveltejs/cli/integration';

export default defineIntegration({
	description: 'An adder template demo',
	environments: { kit: true, svelte: true },
	options: {
		demo: {
			question: 'Do you want to use a demo?',
			type: 'boolean',
			default: false
		}
	},
	files: ({ options, kit }) => [
		{
			name: 'adder-template-demo.txt',
			content: (content) => {
				if (content) return content; // leave alone
				if (options.demo) {
					return 'This is a text file made by the integration template demo!'
				}
		  }
		},
		{
			name: `${kit.routes}/+layout.svelte`,
			condition: !!kit,
			content: (content) => {
				return content || '<slot />';
			}
		},
		{
			name: `${kit.routes}/+page.svelte,
			condition: !!kit,
			content: (content) => {
				const { script, css, generateCode } = parseSvelte(content);
				script.importNamed('../adder-template-demo.txt?raw', { demo: 'Demo' });
				css.addClass('foobar', { 'background-color': 'blue' });
				return generateCode({ script: script.generateCode(), css: css.generateCode() });
			}
		{
	]
});

after authoring a whole bunch of adders, this really does feel like the right level of abstraction.

(For that matter I'm not sure we would need things like api.createFile — people can just use fs)

i think this would be going too 'low level'. adders shouldn't be messing around with fs directly. there's also a few things that we apply to files before they're written to disk (e.g. ensuring they end on a new line and creating the parent directories of those files if they don't already exist)

[Rich-Harris]

if so then that feels messy, especially when it's going to be a super common action

I'm not saying it doesn't warrant special attention, just that this...

setup: ({ api, options }) => {
  api.addDevDependency('tailwindcss', '^3.4.9');
  api.addDevDependency('autoprefixer', '^10.4.20');

  if (options.plugins.includes('typography')) {
    api.addDevDependency('@tailwindcss/typography', '^0.5.14');
  }

  if (options.prettier) {
    api.addDevDependency('prettier-plugin-tailwindcss', '^0.6.5');
  }
}

...feels a lot more flexible and future-proof to me than this:

packages: [
  { name: 'tailwindcss', version: '^3.4.9', dev: true },
  { name: 'autoprefixer', version: '^10.4.20', dev: true },
  {
    name: '@tailwindcss/typography',
    version: '^0.5.14',
    dev: true,
    condition: ({ options }) => options.plugins.includes('typography')
  },
  {
    name: 'prettier-plugin-tailwindcss',
    version: '^0.6.5',
    dev: true,
    condition: ({ prettier }) => prettier
  }
]

You never know what satanic logic people are going to need to implement, and it's a hell of a lot easier for them to do so if they don't need to contort everything into a condition function that may need to be repeated multiple times but could have been a single if.

something like this would be ideal:

There's a bug in your code. If kit is falsy, then `${kit.routes}/+layout.svelte` won't evaluate. To me, that's a red flag, and a prime example of something that would be much nicer to write like this:

setup: ({ file, options, kit }) => {
  if (options.demo) {
    file.createIfNotExists('adder-template-demo.txt', 'This is a text file made by the integration template demo!');
  }

  if (!!kit) {
    file.createIfNotExists(`${kit.routes}/+layout.svelte`, '<slot />');

    file.update(`${kit.routes}/+page.svelte`, (content) => {
      const { script, css, generateCode } = parseSvelte(content);
      script.importNamed('../adder-template-demo.txt?raw', { demo: 'Demo' });
      css.addClass('foobar', { 'background-color': 'blue' });
      return generateCode({ script: script.generateCode(), css: css.generateCode() });
    })
  }
}

It's significantly less code, much easier to follow (to me), and if I want to do something like rename a file then I don't have to figure out what that looks like declaratively (if we're really not keen on letting people use fs directly then yes, people will need to figure out which method to use, but that's a lot easier when you can just type file. and watch the suggestions pop up).

[AdrianGonz97]

I'm not saying it doesn't warrant special attention, just that this...

ahh, okay. that's certainly more reasonable than what I had imagined. i like it!

There's a bug in your code. If kit is falsy, then ${kit.routes}/+layout.svelte won't evaluate. To me, that's a red flag, and a prime example of something that would be much nicer to write like this:

i wrote that entirely outside of the editor, but the lsp would've caught that 😄

It's significantly less code, much easier to follow (to me), and if I want to do something like rename a file then I don't have to figure out what that looks like declaratively

damn, that really is quite clean. alright, I'm convinced!

what do you guys think @benmccann @manuel3108 ?

[benmccann]

Yeah, I agree that packages, scripts, and files could all be combined into a single thing.

I might call it execute rather than setup. setup sounds like something that happens beforehand, but this is really the method where all the work happens

I'd probably import file rather than have it be passed in as it feels like it would give us more flexibility around changing it in the future. If it's imported then different community adders can depend on different major versions of the helper APIs. The adder API itself feels like it needs to be much more locked down

[benmccann]

We're also duplicating the metadata for community adders. It's declared in the adder itself and in https://github.com/sveltejs/cli/tree/main/community-adders

[AdrianGonz97]

I might call it execute rather than setup. setup sounds like something that happens beforehand, but this is really the method where all the work happens

run might be a nicer alternative

We're also duplicating the metadata for community adders. It's declared in the adder itself and in main/community-adders

most of these (with the exception of website, i guess) are necessary for the interactive prompt for community adders. similar to normal adders, we provide links to their repos as hints:

if the metadata was not present in our registry, then we'd have to fetch all of their package.json's before we could present them, which is definitely not ideal. this is also an internal api, so we don't have to be as conservative as we do with our public facing apis

[Rich-Harris]

I'd probably import file rather than have it be passed in as it feels like it would give us more flexibility around changing it in the future

My reasoning was threefold:

• Currently you're not allowed dependencies
• There might be cases where it's useful for the method to have context — for example already having access to the evaluated svelte.config.js (instead of you needing to pass it around, or each helper needing to resolve and evaluate it itself), or maybe we want to track which files are created/removed/renamed with the built-in API. This is speculative though
• Convenience/cohesion (particularly around things like logging). It's how adapters work, where the adapt function gets a builder object with project options and helpers that e.g. generate temporary directories inside .svelte-kit without you having to hardcode ".svelte-kit" anywhere

But yes, there are trade-offs. And I haven't really given any thought into what would go into the API.