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

Allow null on TextArea #1933

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

CordlessWool
Copy link

Allow null as value attribute on TextArea as on TextInput

@@ -8,7 +8,7 @@ export interface TextAreaProps extends RestProps {
* Specify the textarea value
* @default ""
*/
value?: string;
value?: null | string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files are generated and should not to be edited manually.

The property would need a @type {...} annotation in the source code.

For reference, the property in TextInput:

/**
* Specify the input value.
*
* `value` will be set to `null` if type="number"
* and the value is empty.
* @type {null | number | string}
*/
export let value = "";

But, I do not think changing this at all makes sense.

The input only has null to accommodate type="number" (see documentation above).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I first wanted to add it there, but then I was confused how it get the type if there is nothing defined before?

At least for the component it does not make a different if it is null or undefined, doesn't it? To put values in the user mabybe have to change the type or do a cast, without the need to, because the textarea component can handle null values as it do for undefined. That is currently our problem.

* Specify the textarea value.
*
* and the value is empty.
* @type {null | string}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the comment be simplified to:

/**
 * Specify the textarea value
 * @type {null | string}
 */

Copy link
Collaborator

@metonym metonym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it makes sense to loosen the value type to accept null so that it works in TypeScript strict mode.

Could you amend the JSDoc comment and run yarn build:docs? Doing so will auto-generate the corresponding TypeScript definitions.

@brunnerh
Copy link
Contributor

I think that it makes sense to loosen the value type to accept null so that it works in TypeScript strict mode.

If the Svelte language tools were actually strict, this would just create problems elsewhere.
With null in the type, it should not be possible to bind:value to a string variable any more because null is not assignable to string.

If this were to cause errors on all existing bindings, I would be quite opposed to this, but it looks like this is not the case...

@CordlessWool
Copy link
Author

Since when is null related to number, null is not 0. To be 100% sure I also made a short implementation and I didn't had any problems with combining null, undefinded and string in combination with a bind. TS is in strict mode.

@brunnerh
Copy link
Contributor

Since when is null related to number, null is not 0.

That was exactly my point, null is not compatible with other types, which is why such assignments are not allowed by TS itself.
The Svelte language tools (conveniently?) ignore this.

Pure TS example:

let componentProperty: string | null = {} as any;
let variableThatIsBound: string = {} as any;

variableThatIsBound = componentProperty;

Error on variableThatIsBound:

Type 'string | null' is not assignable to type 'string'.
  Type 'null' is not assignable to type 'string'.(2322)

@CordlessWool
Copy link
Author

CordlessWool commented Mar 20, 2024

Yes and exacatly this is why we need null as option because we have a variable that could be null | undefined | string and this is not assign able to undefined | string. To handle undefiend or null will make no difference for the component.

Undefiend will also not assign able to string and anyway it is allowed, so the component already handle it and you will be able to assign string to string | null.

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 this pull request may close these issues.

None yet

3 participants