sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.37k stars 4.1k forks source link

Proposal: Metadata to components for documentation and creation of default behaviors (Svelte3) #2134

Open georges-gomes opened 5 years ago

georges-gomes commented 5 years ago

Why

I believe that documenting components is an important process for the proper re-usability of components. This can only be done if the code is the source of truth I think. That's why I would like to see metadata around the API bits exposed by Svelte components.

Properties

<script>
  export let name = 'world';
</script>

<h1>Hello {name}!</h1>

Properties need the following data to be documented:

Some of these metadata:

Can automatically reject invalid input data in property with very pretty much no code

What is available elsewhere:

Slots

Slots are part of the API of the component exposed to the user/dev.

Slots would need the following metadata at least:

Events

I'm not too familiar with events in Svelte, but again it's part of the API exposed by the component.

CSS custom properties

Maybe optional. OK.

But they go through Shadow DOM fences. Used to style compoments from the outside. Will likely be replace by ::part spec coming up...

georges-gomes commented 5 years ago

Things explored in the Discord room so far:

@required() export let prop1;
import { REQUIRED } from 'svelte';
export let foo = REQUIRED;
export let bar = REQUIRED(String, 'foo must be a string describing the foo')
export let bar2 = OPTIONAL( Number, 42, 'Number of bars');
export const propType = { 
    foo: { required: true, type: String, desc:'my foo desc' }
         bar: { type: Number, desc: 'Number of bars', default: 42 }
}
import { makeRequired } from 'svelte';
export let foo;
makeRequired(foo);
/** Awesome foo
  * @required
  */
export let foo;
georges-gomes commented 5 years ago

For <slots>

...
<!-- Description of the slot here-->
<slot name="slot1">This is default value</slot>
...
georges-gomes commented 5 years ago

For Events

/**
 * Fired when user changed Foo
 * @detail value:String New value of foo
*/
export function fire_customevent_change(details) { 
    // optional code to be executed
};

fire_customevent_change( { value: "" } ); would fire a new CustomEvent('change', { detail: { value: "" } });

/**
 *  When button is pressed
*/
export function fire_customevent_click() { };

<button on:click="fire_customevent_click()"/>
kazzkiq commented 5 years ago

I kinda like the REQUIRED/OPTIONAL methods.

Things seem to get easier to understand and it adds almost no overhead to current declarations.

import { REQUIRED, OPTIONAL } from 'svelte';

export let name = REQUIRED(String, 'Should be a string containing an user name.');

This could also open space for more advanced approaches, like passing a validator function as the "type":

import { REQUIRED, OPTIONAL } from 'svelte';

function LongString(value) {
  if (typeof value !== 'string' || value.toString().length < 5) {
    return false;
  }
  return true;
}

export let name = REQUIRED(LongString, 'Should be a string with at least 5 characters.');

What do you think?

georges-gomes commented 5 years ago

@kazzkiq Like you I think the REQUIRED/OPTIONAL is kind of svelte :)

Maybe a simplified API?

import { PROPERTY } from 'svelte';
export let name = PROPERTY.required(String, 'Should be a string containing an user name.');
export let bar = PROPERTY.optional(Number, 42, 'Should be a number');

I would add the validator as another parameter to not loose the type

import { PROPERTY } from 'svelte';
export let bar = PROPERTY.optional(Number, 42, 'Should be a number', validationFunction );

I think with {PROPERTY} you don't need export anymore.

import { PROPERTY } from 'svelte';
let bar = PROPERTY.optional(Number, 42, 'Should be a number', validationFunction );
let foo = 'not exposed';
chris-morgan commented 5 years ago

Somewhat related: I would really, really like to be able to pass properties with names that are keywords (class instead of className), or not identifiers (aria-foo instead of ariaFoo). I see no reason why the local property name actually needs to correspond to how it will be passed. I imagine something like this:

/* @svelte(name = "class") */ export let className;
/* @svelte(name = "aria-label") */ export let ariaLabel;

One downside is that pass-through can no longer be shorthand:

<Foo aria-label={ariaLabel} class={className}/>

An alternative for the kebab-case part would be simply always turning it into camelCase in component instantiations (so that the last example would be like new Foo({ ariaLabel: ariaLabel, "class": className }), and {aria-label} pass-through could then be permitted too), but that wouldn’t do anything for class. I… hmm, I think I actually wouldn’t be particularly averse to special-casing class as className, which is the main keyword that people will deal with (but {class} as class={className} would be fairly awful, maybe skip that).

Anyway, all this is food for thought, given that it’s around changes of property semantics and associated metadata (the name you’re exposing the property can fairly be considered part of the metadata). For attaching metadata, magic comments are an option just as magic functions are. Magic functions are a dangerous business.

lukeed commented 5 years ago

I'm not entirely sure how I feel about all of this, but it reminds me of Angular's @Component declarations.

I saw some hesitations in chat (probably here too) about this stuff getting in the way of the template itself and/or making everything too noisy and congested.

So I think a good compromise may be to have an opt-in script tag specifically for metadata. Since we can now process multiple scripts per component, might as well use it?

<script type=metadata>
  export const props = {
    name: {
      required: true,
      type: String,
      // ...
    }
  }

  // Other types of things 
</script>
ansarizafar commented 5 years ago

I think a general validation system with function chaining for both props and component data would be very useful and for documentation, we can use javascript comment syntax with some additions as described by @georges-gomes above


<script>
import {validate, errors} from 'svelte/validations';

/*
for documrntatiom
*/
export let foo = validate().type('string').required().default('abc')

let xyz = vallidate().type('int').optional().default(10)

let user = validate().schema({
name: validate().type('string').required(),
age: validate().type('int').between(18, 25)
})

//Custom validator
validate.extend = function(name, ....) {
}

</script>

<input bind:value={user.name}>
{if !$errors.user.name.isValid}
<div>{$errors.user.name.message}</div>
{/if}
georges-gomes commented 5 years ago

@lukeed

+1 for having it out of the way. Because you don't need it "in your face" all the time.

I would maybe write it in ts:

<script lang="ts" type="metadata">
export interface properties {
   /** Multi-line description goes here */
   name: string;

  /** Optional age */
   age ?: number;
}

?: gives optional in typescript interface This gives the require/optional information

georges-gomes commented 5 years ago

@chris-morgan Your point is interesting as it gives decorators without using decorators. Downside is no lint (by default)

georges-gomes commented 5 years ago

@ansarizafar I agree, about the description. Should stay in javascript /** comments */. Keeps them a little bit out of the way and they are documentation more than anything else.

Regarding the validator, I would maybe follow @lukeed idea and have them in the new <script> block. This way the svelte main <script> block stays small and straight to the point. The constraints and rules are done on the side in the best possible way.

<script>
   export let count = 0;
</script>

This is the best readable form for what is important. We can have the rest on the side as @lukeed idea.

pngwn commented 5 years ago

I don't necessarily agree that source code should act as documentation (especially not if it bloats the API), but having an interface that the compiler understands will allow us to write components with a more robust public interface and there are potential tooling benefits there too.

I suggested the TS interface idea yesterday but the more I think about it, the less I like it. It has a number of limitations that will make working with it awkward (no default values, it is TS not JS), even if some of the shorthand syntax is quite nice (some of it is and some of it isn't).

I particularly like @lukeed's sugesstion about using a separate script tag for the metadata. It would be as flexible as we need it to be:

<script type=metadata>
  export const props = {
    name: {
      required: false, // default?
      type: String, 
      default: 'Benjamin', 
      writable: true, // default?
      validator: s => /^[A-Za-z]+$/.test(s) // default v => v
      // ...
    }
  }

  export const events = {
    boing: {
      description: 'An event that does some thing',
      params: {
        foo: {
          type: String,
          required: false, // default?
          description: 'A really important param'
        }
      }
      // ...
    }
  }

  // Other types of things 
</script>

Here we could do things like validators, default props, readable/writable. The syntax is simple, valid javascript. We can export events and props separately (since their behaviour is different) giving us some flexibility regarding APIs for prop vs event definitions.

The benefit of something like events being on the interface is that the compiler could issue warnings if consumers subscribe to events that aren't on the interface. But this may pose some technical problems, svelte would need to map dispatch calls to event interface definitions, I don't know how much of a problem this will be.

georges-gomes commented 5 years ago

@pngwn Music to my ears. the tooling is what I have in mind.

Let me try to flesh it out a little bit v0.1

<script type="metadata">
  export const props = {
    name: {
      description: 'Name of the person', // Default undefined
      required: false, // default false
      type: [String, undefined] // default [String]
       default: 'Benjamin', // default undefined
      writable: true, // default false
      validator: (value) => boolean // default v => true
      linkAttrib: true // default false (this would be for compiled Web Component only)
    }
  }

  export const events = {
    boing: {
      description: 'An event that does some thing',
      params: {. // Should we use 'details' like standard CustomEvent?
        foo: {
          type: [String],
          required: false, // default
          description: 'A really important param'
        }
      }

  export const slots = {
    'namedslot1' : {
       required: true // default false
       description: 'Slot for avatar'
    }
    0: { }
    1: { }
    // Position 0 and 1 of unnamed slot
  }
</script>
georges-gomes commented 5 years ago

would be nice to have some sort of enum description for the type like

type: oneof("small", "medium", "high");
or
type: ["small", "medium", "high"]
georges-gomes commented 5 years ago

Or we extend validator

type: String;
validator: {
  func: s => ["s", "m", "h"].contains(s);
  description: 'One of these values: "s", "m", "h"'
}

This is probably the way to go in order to express more things in "english" like ranges and formats.

pngwn commented 5 years ago

I don't think it would need to be this complex. The validator, if it were accepted, could handle enums of any type as you said and it would not need its own description, the descrition property on the prop itself could give enough information about the type.

We want a nice flexible interface for this but we don't want to drown in complexity or no-one will use it.

georges-gomes commented 5 years ago

One part of me would like a more precise, programmatic description of the possible values/validation because the tooling that can be created around it would be really nice... But I'm sensitive to you point of overwhelming complexity

Food for thoughts: we may want to have the validator to be an object:

{ valid: false, errormsg: 'Must be at least 5 characters', errorcode: '1234' }

To provide a bit more context to the invalidity

swyxio commented 5 years ago

right, i'm not so keen on adding a homecooked version of prop-types, since we should just use typescript for that whenever it eventually lands, however, the issue of getting metadata is VERY handy and i'd love what @georges-gomes proposed here

DrSensor commented 5 years ago

A bit radical, how about markdown?

example ~~~svelte

The answer is {answer}

# ComponentName > short description Long description about the component, mechanism, what it does, caveats, things to watch out, and so on. ## Properties ### answer > short description Long description #### example1 ```svelte ``` #### example2 ```svelte ``` ~~~
rendered # ComponentName > short description Long description about the component, mechanism, what it does, caveats, things to watch out, and so on. ## Properties ### answer - `string | number` - default - `"__not specified__"` - required! - read-only! > short description Long description #### example1 ```svelte ``` #### example2 ```svelte ``` ---

Benefits

Caveats

caveat workaround
prop title need to be retyped in the docs a tool for auto-generate and update prop title of the docs
source and docs can be unsync a linter to check if docs and exported prop are sync
difficult to integrate with jsdoc or tsdoc ??
not implicit
swyxio commented 5 years ago

@DrSensor this helps docs but what about other forms of metadata?

DrSensor commented 5 years ago

I think metadata like Type(s), Validation, and Required should be inferred from the source, especially when written in typescript or use label if it's not written in typescript.

required, read-only: export let answer = [String("__not specified__"), Number]
stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.