language-tools icon indicating copy to clipboard operation
language-tools copied to clipboard

Property with an initial default value can get a undefined excluding TypeScript type while still becoming undefined during runtime

Open janvogt opened this issue 1 year ago • 9 comments

Describe the bug

According to https://github.com/sveltejs/svelte/issues/4442 props' default values are really initial values as they are only used when a component is first created. While I consider this, as well as others who've created issues bevore me, to be a rather unintuitive design decision, I assume there to be good reasons for it.

However, after spending over half a day tracking down this issue from an obscure exception thrown in a production app, I think it remains at least a bug in Svelte's TypeScript implementation. We were able to type an optional property that had an initial value with a type that excludes undefined. However, the value of that property may very much become undefined during runtime.

To be specific, I belive I should not be able do this:

<script lang="ts">
  export let optional: string = "this may be undefined, since this is 'only' an inital value!"
</script>

This is essentially a loaded footgun.

While I rate this as an annoyance, I cannot stress enough how much this defeats the core reason we use TypeScript in the first place.

If this will not be fixed, are there some ways to automatically prevent such flawed uses, i.e. intital values when they really should be a default?

This exact problem has been described in a comment before, as well in this issue in sveltejs language-tools. The reason to close the latter, is pretty murky to me.

Reproduction

This is a short repro simplified from our use case, unfortunately the repl does not support TypeScript. https://svelte.dev/repl/1448697c1adc4f31b658f5207747dece?version=3.29.4

Logs

No response

System Info

System:
    OS: Linux 6.1 NixOS 23.05 (Stoat) 23.05 (Stoat)
    CPU: (4) x64 Intel Xeon Processor (Skylake, IBRS)
    Memory: 10.71 GB / 15.26 GB
    Container: Yes
    Shell: 3.6.4 - /run/current-system/sw/bin/fish
  Binaries:
    Node: 20.9.0 - /nix/store/a1hckfqzyys4rfgbdy5kmb5w0zdr55i5-nodejs-20.9.0/bin/node
    npm: 10.1.0 - /nix/store/a1hckfqzyys4rfgbdy5kmb5w0zdr55i5-nodejs-20.9.0/bin/npm

Severity

annoyance

janvogt avatar Dec 19 '23 22:12 janvogt

This is the same as https://github.com/sveltejs/svelte/issues/9948 but about Svetle 4

7nik avatar Dec 19 '23 22:12 7nik

This is essentially a loaded footgun.

The main problem is, that most people expect and think, this is a default value (like function default value). But in reality this is only an initial value (like class constructor).

And because a lot of people think so (and if they do not know this issue) - here are potentially many svelte apps/sites that can break - any time, if not tested well.

Svelte 4 is based on classes (I think), so this makes more sense... I don't know if this could be changed.

Svelte 5 have a chance to make it right. I hope for this - really! Other frameworks (react, vue, solid-js) have the ability to set a default value.

Today... you have only this 2 ways to WORKAROUND it (and it is very svelte unlike!)

export let myprop = undefined //make it optional with undefined
$: if (myprop===undefined) myprop=123

or... more hard-coded

{myprop || 123}

dm-de avatar Dec 19 '23 22:12 dm-de

Thanks for these quick replies.

To be clear: this issue is not about Svelte's weird initial prop behavior. I understand that this is non negotiable at this point.

This issue is about enforce an appropriate TypeScript type to represent this weird behavior. I believe this will go a long way in mitigating the resulting risks. I for one would have certainly found this Svelte issue way earlier, if TypeScript had complained.

The type of any prop with an initial (a.k.a. default) value must always include undefined, as there is no guarantee to prevent it from assuming undefined during runtime.

Of course even better would be appropriate narrowing, if the developer actually sets a real default via

<script lang="ts">
  // It should be required to do this:
  export let propWithInitalValue: string | undefined = "inital value"
  // currently propWithInitalValue: string is accepted by TypeScript which
  // allows invalid programms to pass type checking.

  // This should continue to work:
  export let propWithRealDefault: string;
  $: propWithRealDefault ??= "default value";
</script>

janvogt avatar Dec 19 '23 23:12 janvogt

I am not sure if you understand the linked language-tools issue correctly. The problem mentioned there is that:

In { a?: string }, a is always string | undefined. so { a: string | undefined } can assign to it. In TypeScript 4.4, a flag exactOptionalPropertyTypes is added so that it can't. And only { a: string } can. Making optional props always possibly undefined in the component definition means you can no longer use exactOptionalPropertyTypes to enforce it on the usage side.

exactOptionalPropertyTypes: true https://www.typescriptlang.org/play?exactOptionalPropertyTypes=true#code/PTAEAEGcBcCcEsDG0Bco4FcCmAoEEsAPAQ2QHkAHaeAewDtiAbABVhoq1mgE8AVbjpDSZciejFAAjNAG9QxAPxoYCOgHNQAX1ABeUHOJoMdACZYAZvDpYTWnEA

exactOptionalPropertyTypes: false https://www.typescriptlang.org/play?#code/PTAEAEGcBcCcEsDG0Bco4FcCmAoHiB7AOxlAEM0BvcgfjRgSIHNQBfUAXlGotAyIAmWAGbwiWAW1B4gA

jasonlyu123 avatar Dec 20 '23 01:12 jasonlyu123

I hope I understood that correclty @jasonlyu123. You are concerned with the following scenario:

Given a Component.svelte with a property prop that is marked as optional by providing an initial value.

<script lang="ts">
  export let prop: string = "inital value"

  $: () => { 
      if (prop === undefined) { 
        // Dead code, as prop is always a string. Thanks, TypeScript
        destroyTheWorld() 
      }
    }
</script>

A SafeConsumer.svelte should only be allowed to do the first two calls, but not the third:

<script lang="ts">
  import Component from './Component.svelte'
</script>

<Component prop="A real value" />
<Component />
<!-- the following must be an error, as it would destroyTheWorld() -->
<Component prop={undefined} />

Explicitely setting prop to undefined must be an error and only ommiting prop should be accepted. That way, the initial value will be used.

The idea behind this is to guarantee prop never becomes undefined: Either a string is provided or the property will never be set and it's initial value is used.

Unfortunetely, this does not work: Consider this DoomConsumer.svelte:

<script lang="ts">
  import Component from './Component.svelte'

  let counterExample: { prop?: string } = { prop: "defined" }
</script>

<Component {...counterExample} />
<button on:click={() => {
    // nothing to complain here:
    counterExample = {}
  }
}>Break gurantee!</button>

So, within our Component.svelte the type string for prop is unfortunately untrue, as it very well might become undefined. In this example, this would have dire consequences.

janvogt avatar Dec 20 '23 12:12 janvogt

Let's wait with proceeding this until we have decided if we want to switch the default behavior in Svelte 5.

dummdidumm avatar Jan 02 '24 09:01 dummdidumm

Let's wait with proceeding this until we have decided if we want to switch the default behavior in Svelte 5.

Is there an update on this issue now that Svelte 5 is further along in it's implementation?

PascalHonegger avatar Mar 01 '24 13:03 PascalHonegger

Honestly, I would say this is more than just an annoyance. When you're just working on your own code, it's an annoyance. When you're working with libraries that aren't aware of this issue, it's a proper minefield. Could I go in and submit a PR for everything I come across? Sure. But I'm sure that 99% of the issues are because Svelte made a choice that is unwise in retrospect.

https://svelte.dev/repl/b71c5df8c24c4221a1c8b13184455375?version=3.46.6

alshain avatar Mar 01 '24 13:03 alshain

My "solution" today is:

  1. I use default definitions where possible
  2. If I pass a variable to prop, and if this variable can be undefined, I add same default (double code!) on top of that: <Child value={myvalue || 'default'} />

dm-de avatar Mar 01 '24 16:03 dm-de

Svelte 5 will use the fallback value everytime the prop becomes undefined, therefore closing

dummdidumm avatar Jul 31 '24 12:07 dummdidumm