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

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

Closed
janvogt opened this issue Dec 19, 2023 · 16 comments
Labels
Blocked Cannot proceed until something else is resolved

Comments

@janvogt
Copy link

janvogt commented Dec 19, 2023

Describe the bug

According to sveltejs/svelte#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 janvogt changed the title Property with an initial default value can get a undefined excluding TypeScript type while still become undefined during runtime Property with an initial default value can get a undefined excluding TypeScript type while still becoming undefined during runtime Dec 19, 2023
@7nik
Copy link

7nik commented Dec 19, 2023

This is the same as sveltejs/svelte#9948 but about Svetle 4

@dm-de
Copy link

dm-de commented Dec 19, 2023

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}

@janvogt
Copy link
Author

janvogt commented Dec 19, 2023

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>

@jasonlyu123
Copy link
Member

jasonlyu123 commented Dec 20, 2023

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

@janvogt
Copy link
Author

janvogt commented Dec 20, 2023

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.

@jasonlyu123 jasonlyu123 transferred this issue from sveltejs/svelte Dec 21, 2023
@dummdidumm
Copy link
Member

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

@dummdidumm dummdidumm added the Blocked Cannot proceed until something else is resolved label Jan 2, 2024
@PascalHonegger
Copy link

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?

@alshain
Copy link

alshain commented Mar 1, 2024

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

@dm-de
Copy link

dm-de commented Mar 1, 2024

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'} />

@dummdidumm
Copy link
Member

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

@janvogt
Copy link
Author

janvogt commented Aug 25, 2024

This issue was explicitly not about Svelte 4's default behaviour but about the language-tools wrong TypeScript type within a component. I don't get what this - certainly laudable - change in Svelte 5 fixes about this issue?

@dummdidumm
Copy link
Member

dummdidumm commented Aug 25, 2024

Because the runtime behavior in Svelte 5 using the $props rune matches the TS type definition

@janvogt
Copy link
Author

janvogt commented Aug 25, 2024

I got this. However: Svelte 5 is not released. Svelte 4 or even 3 codebases are pretty much a thing, and IMHO will be for quite a while. Nothing has changed for them regarding this issue.

If the reason for closing is that language-tools wont support anything less than svelte 5 anymore, this should be at least closed as wontfix instead of completed. Otherwise it's very misleading.

@dummdidumm
Copy link
Member

It's closed as in fixed in Svelte 5. We won't/can't backport this as changing this would be a breaking change

@janvogt
Copy link
Author

janvogt commented Sep 6, 2024

I feel, as the OP of this issue, it should have some weight, when I say, that this is not resolved. Of course, it's your project and you are free to close any issue you want even without actually adressing them.

With Svelte 5 this issue might not exist anymore, but it's certainly not fixed.

Tbc: As explicitely stated earlier this issue is not with the weird behaviour around default values that Svelte <5 exhibits, but about the language tools using wrong types in many svelte projects. Having the right types in Svelte 5 projects is cool, but not a fix.

But yaeh, I will not continue to argue any further. It is what it is.

@dummdidumm
Copy link
Member

As stated earlier, this would be a huge breaking change (many people's projects would be littered with type errors), which is why we're not fixing this for Svelte 4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Cannot proceed until something else is resolved
Projects
None yet
Development

No branches or pull requests

7 participants