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

Module-level binding in svelte 3.16 #4086

Closed
rootasjey opened this issue Dec 10, 2019 · 12 comments · Fixed by #4352
Closed

Module-level binding in svelte 3.16 #4086

rootasjey opened this issue Dec 10, 2019 · 12 comments · Fixed by #4352
Assignees
Labels

Comments

@rootasjey
Copy link

Hi, after I updated svelte to the version 3.16.0, the build command doesn't work anymore.

Modules versions:

  • "svelte": "3.16.0"
  • "rollup-plugin-svelte": "5.1.1",

Output:

⟩ yarn run build
yarn run v1.19.1
$ rollup -c

src/main.js → public/bundle.js...
[!] (plugin svelte) TypeError: Cannot read property 'index' of undefined
src/components/Header.svelte
TypeError: Cannot read property 'index' of undefined
    at Renderer.invalidate (/Users/jeremiecorpinot/GitHub/memorare/front-app/node_modules/svelte/src/compiler/compile/render_dom/Renderer.ts:169:35)
    at bind_this (/Users/jeremiecorpinot/GitHub/memorare/front-app/node_modules/svelte/src/compiler/compile/render_dom/wrappers/shared/bind_this.ts:32:22)
    at ElementWrapper.add_bindings (/Users/jeremiecorpinot/GitHub/memorare/front-app/node_modules/svelte/src/compiler/compile/render_dom/wrappers/Element/index.ts:592:29)
    at ElementWrapper.render (/Users/jeremiecorpinot/GitHub/memorare/front-app/node_modules/svelte/src/compiler/compile/render_dom/wrappers/Element/index.ts:380:8)
    at FragmentWrapper.render (/Users/jeremiecorpinot/GitHub/memorare/front-app/node_modules/svelte/src/compiler/compile/render_dom/wrappers/Fragment.ts:151:18)
    at new Renderer (/Users/jeremiecorpinot/GitHub/memorare/front-app/node_modules/svelte/src/compiler/compile/render_dom/Renderer.ts:95:17)
    at dom (/Users/jeremiecorpinot/GitHub/memorare/front-app/node_modules/svelte/src/compiler/compile/render_dom/index.ts:17:19)
    at render_dom (/Users/jeremiecorpinot/GitHub/memorare/front-app/node_modules/svelte/src/compiler/compile/index.ts:97:6)
    at /Users/jeremiecorpinot/GitHub/memorare/front-app/node_modules/rollup-plugin-svelte/index.js:252:22

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

What's in the file:
Header.svelte

<script context="module">
  let domHeader
  // ...other variables

  export function hideHeader() {
   // ...function's body
  }
</script>

<script>
  // ...some JavaScript code
</script>

<style>
  /*...some styles */
</style>

<header>
  <!-- some dom -->
</header>

To see the full code you can go to my repo Header.svelte

The same code works with the svelte version 3.15.0
The bug appears when I require the exported functions in the script module with svelte 3.16.0.

Example:

// Some other module

import { hideHeader } from 'Header.svelte';
@Conduitry Conduitry transferred this issue from sveltejs/rollup-plugin-svelte Dec 10, 2019
@antony
Copy link
Member

antony commented Dec 10, 2019

@rootasjey What did you upgrade from? What's the latest version that works?

Is it possible to create a REPL to demonstrate the issue?

@Conduitry
Copy link
Member

The root of the matter seems to be:

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

<div bind:this={foo}/>

I haven't checked whether this did the right thing in 3.15.0. It's somewhat weird having a binding to a module-level variable (I guess all of the instances of the component would just overwrite one another?)

@Conduitry Conduitry added the bug label Dec 10, 2019
@ghost
Copy link

ghost commented Dec 10, 2019

@Conduitry, 3.15.0 compiles OK, at least according to REPL.
https://svelte.dev/repl/77e6667473c34065a02e2ab675343107?version=3.15.0

@Conduitry
Copy link
Member

I see that it compiles okay - what I haven't checked is whether the code does the right thing.

@rootasjey
Copy link
Author

I haven't checked whether this did the right thing in 3.15.0. It's somewhat weird having a binding to a module-level variable (I guess all of the instances of the component would just overwrite one another?)

Yeah I thought I was doing something not-classy when writing this (the module-level binding).
I can find a workaround now that you pointed the issue :)

Thanks!

@rootasjey rootasjey changed the title [Bug?] Build fails with svelte 3.16 Module-level binding in svelte 3.16 Dec 10, 2019
@anlexN
Copy link

anlexN commented Dec 11, 2019

svelte component script natively is module, why use syntax: "<script context="module">" ? I am a fresh man, Am I misunderstood svelte official tutorial?

@antony
Copy link
Member

antony commented Dec 11, 2019

@ghost
Copy link

ghost commented Dec 18, 2019

@Conduitry,

In 3.15, it has the last in behavior you'd expect (though probably not a good thing)... every button in the repl below prints "Third".

https://svelte.dev/repl/623d6db3ad1242208a191c5ac1a411fc?version=3.15.0

The invalidate routine is much simpler in 3.15 than 3.16.5... for all the tests in the routine, we still miss member being undefined.

Here's some devtools info:
image

variable = {
hoistable: true
module: true
name: "foo"
reassigned: true
referenced: true
writable: true
}

@vipero07
Copy link

vipero07 commented Jan 2, 2020

Maybe I don't understand the use case here, but shouldn't this just be a store instead of a module level variable?

@Conduitry
Copy link
Member

I think the broader problem here is that we shouldn't try to generate invalidation code for any module-level variables (unless we're updating it as a store). This:

diff --git a/src/compiler/compile/render_dom/Renderer.ts b/src/compiler/compile/render_dom/Renderer.ts
index 046a90b68..be9e700a0 100644
--- a/src/compiler/compile/render_dom/Renderer.ts
+++ b/src/compiler/compile/render_dom/Renderer.ts
@@ -160,6 +160,10 @@ export default class Renderer {
 			return x`${name.slice(1)}.set(${value || name})`;
 		}
 
+		if (variable && variable.module) {
+			return value;
+		}
+
 		if (
 			variable &&
 			!variable.referenced &&

seems to generate sensible code from my example component above, and doesn't break any existing tests.

@Conduitry
Copy link
Member

Additionally, we probably shouldn't be checking the dirty bitmask for changes to module-level variables, nor should these variables even be given a bit in the bitmask, but I don't have quick fixes for those yet.

@Conduitry
Copy link
Member

Binding to a module-level variable no longer causes a compiler error in 3.18.2.

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

Successfully merging a pull request may close this issue.

5 participants