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

The required props are generated as optional #22

Closed
illright opened this issue Jan 16, 2021 · 19 comments · Fixed by #91
Closed

The required props are generated as optional #22

illright opened this issue Jan 16, 2021 · 19 comments · Fixed by #91

Comments

@illright
Copy link

illright commented Jan 16, 2021

If we have the following component:

<script>
  /**
   * @type {string}
   */
  export let a;
</script>

it has a required prop a, but the generated prop types mark it as optional:

export interface ComponentProps {
  a?: string;
}

Is this inteded behaviour? I imagine it may be misleading for the users of typings to not know which props are required.

@illright illright changed the title The required props are generated to be nullable The required props are generated as optional Jan 16, 2021
@metonym
Copy link
Collaborator

metonym commented Jan 16, 2021

This is intentional, albeit a matter of taste.

In our experience with porting Carbon to Svelte, it's annoying to get an error for every possible prop if it's required:

<Button
  icon="{undefined}"
  hasIconOnly={false}
  tooltipPosition="bottom"
  tooltipAlignment="center"
  iconDescription="{undefined}"
  kind="primary"
  as={false}
/>

// TSError! prop "size" cannot be undefined

In your use case, is there any reason why you can't initialize a prop with a default value?

@aabounegm
Copy link
Contributor

aabounegm commented Jan 16, 2021

I don't really get it. Why wouldn't you want an error if you don't pass a required prop? If it has a default value, it is optional. If it doesn't, you must pass a value.
Also, just wanted to point out that

interface Props {
  a?: string;
}

is not the same as

interface Props {
  a: string | undefined;
}

In the first variant, you are allowed to not pass the prop, but if you do then it must be a string (cannot be undefined). In the second one, you must pass something, and that thing can be undefined or a string.
There is a difference between optional and nullable.
(note that by "nullable" here I mean null or undefined, as in the same idea applies to both)

@metonym
Copy link
Collaborator

metonym commented Jan 16, 2021

I think we can agree to disagree.

@illright
Copy link
Author

@metonym in my use case the prop is a function used for generating items. There can be no default for it and the component practically doesn't make sense without it. I understand the usecase with Carbon, but perhaps this could be made as a parameter to the Sveld compiler?

@illright
Copy link
Author

And I must agree with @aabounegm that in the case of Carbon, the typing "sm" | "xl" | undefined would be clearer to convey the meaning that there are three different sizes, not two, as the prop values indicate (before you mentioned it, I literally believed that it's either sm or xl). Then the prop declaration would have to get an explicit default value of undefined and Sveld recognizes this prop as optional and throws no TS errors on its omission.

@aabounegm
Copy link
Contributor

@metonym I should also add that I just check the source code of the Button component in the example you gave above, and the prop is indeed optional (as it has a default value of "default") and cannot be undefined, in which case it makes perfect sense to mark its type as

interface Props {
    size?: "default" | "field" | "small"
}

This way, it can be omitted but cannot be provided as undefined.
The use case we are referring to here is different. Any prop that doesn't have a default value is a required prop, in which case it doesn't make sense to mark it as optional. The 2 snippets I provided have different meanings and are not just different ways of representing the same thing. You can still have a nullable required prop if you want, and it is not the same as not passing a value at all.

@metonym
Copy link
Collaborator

metonym commented Feb 3, 2021

I understand what a nullable type is. I think we differ in how we design component APIs.

@illright
Copy link
Author

illright commented Feb 3, 2021

@metonym would you be interested in a PR that implements another vision behind a config option?

@metonym
Copy link
Collaborator

metonym commented Feb 3, 2021

@illright I'd be open to that, as long as it's opt-in

@ZerdoX-x
Copy link

ZerdoX-x commented Sep 12, 2021

This is important to me. I am creating some modal component that must accept unique id as prop.

My usecase:

Input:

<script lang="ts">
export let id: string
export let closeButtonLabel: string | undefined = undefined
export let alertDialog: boolean = false
// ...

Output:

export interface defaultProps {
  id?: undefined;

  closeButtonLabel?: undefined;

  /**
   * @default false
   */
  alertDialog?: boolean;
}

export default class extends SvelteComponentTyped<
  defaultProps,
  {
    show: CustomEvent<any>;
    hide: CustomEvent<any>;
    destroy: CustomEvent<any>;
    create: CustomEvent<any>;
  },
  { default: {}; ["close-button"]: {}; title: {} }
> {}

@metonym
Copy link
Collaborator

metonym commented Sep 12, 2021

This is important to me. I am creating some modal component that must accept unique id as prop.

@ZerdoX-x Have you considered setting a default value that is randomly generated?

export let id = "id-" + Math.random().toString(36);

@ZerdoX-x
Copy link

ZerdoX-x commented Sep 12, 2021

Have you considered setting a default value that is randomly generated?

Nah. Users will need to define this id by themselves. Let's don't change my usecase. I need this prop to be required, that's it.

@metonym
Copy link
Collaborator

metonym commented Sep 12, 2021

@ZerdoX-x What approach do you suggest? Maybe having a @required tag to mark required tags?

@ZerdoX-x
Copy link

ZerdoX-x commented Sep 12, 2021

Input:

export let id: string
export let closeButtonLabel: string | undefined = undefined

Expected output:

id: string;
/**
 * @default undefined
 */
closeButtonLabel?: string | undefined; 

Actual output:

id?: undefined;
closeButtonLabel?: undefined;

@metonym
Copy link
Collaborator

metonym commented Sep 12, 2021

Currently, sveld does not actually parse TypeScript types. It relies on plain Svelte syntax coupled with JSDocs-style annotations to generate the definitions.

@metonym
Copy link
Collaborator

metonym commented Sep 12, 2021

@ZerdoX-x Have you tried the following alternatives?

  • svelte2tsx
  • svelte-types-writer
  • svelte2dts

metonym added a commit that referenced this issue May 19, 2022
* feat: support `@required` tag to denote required prop (#22)

* Run "yarn test:snapshot"

* test(snapshots): add required props test case

* chore: do not include `@required` tag in TS definitions

* Run "yarn test:integration"

* test(integration): add `@required` prop to Dropdown

* Run "yarn test:integration"

* docs: add `@required`
@metonym
Copy link
Collaborator

metonym commented May 20, 2022

Required props are supported in v0.16.0.

Use the non-standard @required to annotate props that should be required.

Input

<script>
  /**
   * This is a comment.
   * @required
   * @type {boolean | string}
   */
  export let prop1 = true;
</script>

Output

/// <reference types="svelte" />
import type { SvelteComponentTyped } from "svelte";

export interface ComponentProps {
  /**
   * This is a comment.
   * @default true
   */
  prop1: boolean | string;
}

export default class Input extends SvelteComponentTyped<
  ComponentProps,
  {},
  { }
> {}

@aabounegm
Copy link
Contributor

That's great, but doesn't this way clash with the Svelte way/semantics of making a prop required? In Svelte, a prop is automatically optional if it has a default value and required otherwise, so what would it mean to mark a prop with a default value as @required? Does that mean it always has to get passed a value for that prop, rendering the default value useless?
On the other hand, what if a prop did not have a default value (export let prop;) and was not marked as @required, would sveld still mark it as optional (even though Svelte will warn that it wasn't provided)?

metonym added a commit that referenced this issue May 21, 2022
metonym added a commit that referenced this issue May 21, 2022
)

* fix: uninitialized props should be required

Fixes #22

* Run "yarn test:snapshot"

* chore: prop is required if kind is `let` and init is `null`

* breaking: remove `@required` tag

* Run "yarn test:snapshot"

* Run "yarn test:integration"
@metonym
Copy link
Collaborator

metonym commented May 21, 2022

@aabounegm You're correct. Uninitialized export let props should be inferred as required. Fixed in v0.17.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants