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

Erroneous "Stores must be declared at the top level of the component" error #5162

Closed
Rich-Harris opened this issue Jul 17, 2020 · 6 comments
Closed

Comments

@Rich-Harris
Copy link
Member

Describe the bug
Since #5079, valid code is causing the following error:

Stores must be declared at the top level of the component (this may change in a future version of Svelte)

To Reproduce

https://svelte.dev/repl/8240f1807eda4d429018826229910e48?version=3.24.0

It's coming from this code:

<script>
  import { getChartContext } from './Chart.svelte';
  import { get_ticks } from '../utils/ticks.mjs';
  
  // ...

  const { x1, y1, x2, y2, x, y } = getChartContext();
  
  // ...

  $: style = orientation === HORIZONTAL
    ? (y, i) => `width: 100%; height: 0; top: ${$y(y, i)}%`
    : (x, i) => `width: 0; height: 100%; left: ${$x(x, i)}%`;
</script>

The y that is part of the chart context is a store; the y inside the closure is a number.

Expected behavior
No error

Severity

Moderate. Requires either downgrading Svelte in our project, or working around the bug in Pancake

@Rich-Harris
Copy link
Member Author

Ah — looks like someone caught this a few days ago Rich-Harris/pancake#19

@Conduitry
Copy link
Member

Yeah I tried to solicit input on #5079. It is technically a breaking change, but I'd argue that the code that is now disallowed was confusing. I'd kind of push for changing components that were affected by this, rather than rolling back part of this check. Having a scope where foo and $foo refer to different variables seems quite confusing.

@Rich-Harris
Copy link
Member Author

Yeah, I guess in this situation we get to decide whether the code is 'valid' or not. For the time being at least I've made the change in pancake. I guess I wouldn't be put out if we closed this accordingly; at least there's an issue to land on if someone searches for the text of the error now.

@Conduitry
Copy link
Member

Something else I had mentioned on that other PR - if we ever plan to allow subscribing to stores that are not defined at the top-level, that would have itself technically been a breaking change vs. the old behavior. (Because using $foo in a scope where foo was shadowing a top-level foo would suddenly mean something different.) Having a nice long intervening period where it throws a compile time error seems a lot safer.

@Myrmod
Copy link

Myrmod commented May 5, 2022

A problem I had was that I wanted to use the values of stores that have been bound by components. The solution for getting the values of the stores is using

import { get } from 'svelte/store'

function getValue(store) {
  // works
  return get(store)
  
  // doesn't work
  return $store
}
...

I made an example of my use case in a repl: https://svelte.dev/repl/acccf5911f7e41af8025ee88fc005a56?version=3.48.0

@Rich-Harris
Copy link
Member Author

Closing as $state(...) solves this problem without ambiguity

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

No branches or pull requests

4 participants