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

warn module variables are not reactive and make them truely non reactive #5847

Merged
merged 6 commits into from Jan 29, 2021

Conversation

tanhauhau
Copy link
Member

Fixes #5843

  • added warning when using module variables as reactive declaration dependencies
  • remove module variable as reactive declaration dependencies, making it the variable truly non-reactive.
    • Fixing the inconsistent behavior as seen in this REPL

Before submitting the PR, please make sure you do the following

- [ ] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs

  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@Conduitry
Copy link
Member

What do you mean by making them truly non-reactive? What behavior is changing here besides adding the compiler warning?

@tanhauhau
Copy link
Member Author

so, if u look at the REPL,

the value of a is undefined, where b is "moduleB".

if you look at the compiled code, svelte tries to set the value of a inside the $$self.$$.update, and when it checks that if moduleA is dirty before setting a.

but, moduleA is not an instance variable, it will never be dirty, therefore, the value of a is undefined.

moduleB on the other hand is different, it will not change, therefore, the $: b = moduleB is treated as static reactive declaration, and the value of b is "moduleB".

so i suggest that, if the reactive declaration depends only on module variable, then it should be a static reactive declaration.

@dummdidumm
Copy link
Member

Is that technically a breaking change?
Also: some people might know this and still use module variables in the context of reactive assignments (knowing that something else triggers the recalculation). Is it possible to silence this warning like you can silence other warnings?

@oodavid
Copy link

oodavid commented Jan 2, 2021

@tanhauhau does this PR cover the following? tested in 3.31.0

<script context="module">
  let foo;
</script>
<script>
  let foo;
</script>

In vanilla html you'd see Uncaught SyntaxError: Identifier 'foo' has already been declared

@Conduitry Conduitry merged commit 6589aa2 into sveltejs:master Jan 29, 2021
@Conduitry
Copy link
Member

This has been released in 3.32.1 - https://svelte.dev/repl/7eb62137060d4d53abe5e6a8b45074d5?version=3.32.1

@istarkov
Copy link

istarkov commented Feb 2, 2021

Is it possible to disable this rule? As of now now impossible to reuse imported constants because of this warning.
ie.

<script context="module" lang="ts">
  import { CIRCLE_GAP } from '../../components/carousel/consts';
  console.info(CIRCLE_GAP);
</script>

<script lang="ts">
  import Filters from '../../components/filters.svelte';
  $: {
    console.info(CIRCLE_GAP);
  }
</script>

gives "CIRCLE_GAP" is declared in a module script and will not be reactive.

Ok, constants can be not reactive.

@istarkov
Copy link

istarkov commented Feb 2, 2021

Same for heavy initializers at module level, how I can now init once something without this warning.

@istarkov
Copy link

istarkov commented Feb 2, 2021

I thought this should work <!-- svelte-ignore module-script-reactive-declaration -->,
but not.

@trmcnvn
Copy link

trmcnvn commented Feb 6, 2021

This warning also occurs when using exported functions.

https://svelte.dev/repl/7ddb4af47bbf4cdebbefcf5649e83ff1?version=3.32.1

@j3rem1e
Copy link

j3rem1e commented Feb 6, 2021

I have now a lot of warnings because of this rule.. Please add something to disable it globally.

@dummdidumm
Copy link
Member

For svelte-check, do --compiler-warnings "module-script-reactive-declaration:ignore". A similar option is available for the VS Code extension.
I opened #5954 for a possibility to silence this through a comment.

hontas added a commit to hontas/svelte that referenced this pull request Feb 11, 2021
* 'master' of https://github.com/sveltejs/svelte: (129 commits)
  -> v3.32.3
  fix remove of lone :host selectors (sveltejs#5984)
  -> v3.32.2
  update changelog
  fix extra invalidation with component prop binding to object property (sveltejs#5890)
  update css-tree@^1.1.2 (sveltejs#5958)
  fix :host and :global css scoping (sveltejs#5957)
  Tutorial : a better explanation of component events (sveltejs#4639)
  "What's new in Svelte" February newsletter (sveltejs#5925)
  Change color on 404 page (sveltejs#5932)
  -> v3.32.1
  warn module variables are nonreactive and make them truly nonreactive (sveltejs#5847)
  update changelog
  fix: "foreign" namespace elements should still allow binding 'this' (sveltejs#5942)
  inline `prop_values` in `init` helper (sveltejs#5909)
  update changelog
  don't create class update functions when dependencies aren't reactive (sveltejs#5926)
  fix extraneous store subscription in SSR (sveltejs#5929)
  make `SvelteComponentDev` typings more forgiving (sveltejs#5937)
  make animation/transition params optional (sveltejs#5936)
  ...
@tanhauhau tanhauhau deleted the tanhauhau/gh-5843 branch July 9, 2021 00:41
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.

Module Declarations are not Reactive - is this expected?
7 participants