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

Bind in nested component #714

Closed
stalkerg opened this issue Jul 20, 2017 · 6 comments
Closed

Bind in nested component #714

stalkerg opened this issue Jul 20, 2017 · 6 comments

Comments

@stalkerg
Copy link
Contributor

stalkerg commented Jul 20, 2017

Example: https://svelte.technology/repl?version=1.25.0&gist=ef7e0e85bebbe18bcfb53ffd7e9ebc44
My explanation:
during creating component we have the code:

if (options.target) {
  this._fragment.create();
  this._fragment.mount(options.target, null);
}

if we work with nested component target is undefined and DOM will not create on this stage.
Several lines below we call bindings

callAll(this._bindings); 

but without DOM and will get the error.

@stalkerg stalkerg changed the title Yield in nested component Bind in nested component Jul 20, 2017
@stalkerg
Copy link
Contributor Author

Problem worse because exists without yield too:
https://svelte.technology/repl?version=1.25.0&gist=660895060e64a52f62409adb0254fb21

@stalkerg
Copy link
Contributor Author

stalkerg commented Jul 20, 2017

This patch fix my problem but break runtime/bindings-before-oncreate test (and behaviour)

diff --git a/src/generators/dom/Block.ts b/src/generators/dom/Block.ts
index ff58a4e..e7e7940 100644
--- a/src/generators/dom/Block.ts
+++ b/src/generators/dom/Block.ts
@@ -46,6 +46,7 @@ export default class Block {
 		claim: CodeBuilder;
 		hydrate: CodeBuilder;
 		mount: CodeBuilder;
+		bindings: CodeBuilder;
 		intro: CodeBuilder;
 		update: CodeBuilder;
 		outro: CodeBuilder;
@@ -92,6 +93,7 @@ export default class Block {
 			claim: new CodeBuilder(),
 			hydrate: new CodeBuilder(),
 			mount: new CodeBuilder(),
+			bindings: new CodeBuilder(),
 			intro: new CodeBuilder(),
 			update: new CodeBuilder(),
 			outro: new CodeBuilder(),
@@ -257,6 +259,7 @@ export default class Block {
 			properties.addBlock(deindent`
 				mount: function ( #target, anchor ) {
 					${this.builders.mount}
+					${this.builders.bindings}
 				},
 			`);
 		}
diff --git a/src/generators/dom/index.ts b/src/generators/dom/index.ts
index 55c6599..5efb963 100644
--- a/src/generators/dom/index.ts
+++ b/src/generators/dom/index.ts
@@ -228,7 +228,7 @@ export default function dom(
 			}
 			
 			${generator.hasComponents && `@callAll(this._oncreate);`}
-			${generator.hasComplexBindings && `@callAll(this._bindings);`}
+			${generator.hasComplexBindings && `options.target && @callAll(this._bindings);`}
 
 			${templateProperties.oncreate && deindent`
 				if ( options._root ) {
diff --git a/src/generators/dom/visitors/Component/Component.ts b/src/generators/dom/visitors/Component/Component.ts
index 7145f5d..a5bffec 100644
--- a/src/generators/dom/visitors/Component/Component.ts
+++ b/src/generators/dom/visitors/Component/Component.ts
@@ -232,6 +232,9 @@ export default function visitComponent(
 	block.builders.mount.addLine(
 		`${name}._fragment.mount( ${targetNode}, ${anchorNode} );`
 	);
+	block.builders.bindings.addLine(
+		`@callAll( ${name}._fragment._bindings );`
+	);
 
 	if (!local.update.isEmpty()) block.builders.update.addBlock(local.update);
 }

@stalkerg
Copy link
Contributor Author

stalkerg commented Jul 20, 2017

Maybe it is a bad idea to try to get states from nested components in oncreate function. Anyway, you can ask component directly.
UPDATE
I think much better will be if we just stop update DOM (component.update) in this situation because in any case after we create DOM from actual data. But I can't implement that without sending extra var for many functions...

@stalkerg
Copy link
Contributor Author

stalkerg commented Jul 20, 2017

Second solution, maybe much better without new broken test:

diff --git a/src/generators/dom/index.ts b/src/generators/dom/index.ts
index 55c6599..09b54e8 100644
--- a/src/generators/dom/index.ts
+++ b/src/generators/dom/index.ts
@@ -123,7 +123,8 @@ export default function dom(
 		${computations.length &&
 			`@recompute( this._state, newState, oldState, false )`}
 		@dispatchObservers( this, this._observers.pre, newState, oldState );
-		${block.hasUpdateMethod && `this._fragment.update( newState, this._state );`}
+
+		${block.hasUpdateMethod && `withoutDomUpdate || this._fragment.update( newState, this._state );`}
 		@dispatchObservers( this, this._observers.post, newState, oldState );
 		${generator.hasComponents && `@callAll(this._oncreate);`}
 		${generator.hasComplexBindings && `@callAll(this._bindings);`}
@@ -213,6 +214,7 @@ export default function dom(
 
 			this._fragment = @create_main_fragment( this._state, this );
 
+			this._protectDomUpdate = false;
 			if ( options.target ) {
 				${generator.hydratable
 					? deindent`
@@ -225,10 +227,13 @@ export default function dom(
 						this._fragment.create();
 					`}
 				this._fragment.${block.hasIntroMethod ? 'intro' : 'mount'}( options.target, null );
+			} else {
+				this._protectDomUpdate = true;
 			}
 			
 			${generator.hasComponents && `@callAll(this._oncreate);`}
 			${generator.hasComplexBindings && `@callAll(this._bindings);`}
+			this._protectDomUpdate = false;
 
 			${templateProperties.oncreate && deindent`
 				if ( options._root ) {
@@ -242,7 +247,7 @@ export default function dom(
 
 		@assign( ${prototypeBase}, ${proto});
 
-		${name}.prototype._set = function _set ( newState ) {
+		${name}.prototype._set = function _set ( newState, withoutDomUpdate ) {
 			${_set}
 		};
 
diff --git a/src/generators/dom/visitors/shared/binding/getSetter.ts b/src/generators/dom/visitors/shared/binding/getSetter.ts
index 784e1d7..cfbe591 100644
--- a/src/generators/dom/visitors/shared/binding/getSetter.ts
+++ b/src/generators/dom/visitors/shared/binding/getSetter.ts
@@ -29,10 +29,10 @@ export default function getSetter({
 			${computed
 				? `#component._set({ ${dependencies
 						.map((prop: string) => `${prop}: state.${prop}`)
-						.join(', ')} });`
+						.join(', ')} }, #component._protectDomUpdate);`
 				: `#component._set({ ${dependencies
 						.map((prop: string) => `${prop}: #component.get( '${prop}' )`)
-						.join(', ')} });`}
+						.join(', ')} }, #component._protectDomUpdate);`}
 		`;
 	}
 
@@ -42,11 +42,11 @@ export default function getSetter({
 			${snippet} = ${value};
 			#component._set({ ${dependencies
 				.map((prop: string) => `${prop}: state.${prop}`)
-				.join(', ')} });
+				.join(', ')} }, #component._protectDomUpdate);
 		`;
 	}
 
-	return `#component._set({ ${name}: ${value} });`;
+	return `#component._set({ ${name}: ${value} }, #component._protectDomUpdate);`;
 }
 
 function isComputed(node: Node) {

I can make tests and PR if my approach will be accepted.

stalkerg pushed a commit to stalkerg/svelte that referenced this issue Jul 21, 2017
@Rich-Harris
Copy link
Member

It looks like this is fixed by #709 (i.e. it's the same basic issue as #708), so I'm going to focus on finishing that PR

@Rich-Harris Rich-Harris mentioned this issue Jul 24, 2017
@stalkerg
Copy link
Contributor Author

Yep, this issue fixed.

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

No branches or pull requests

2 participants