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

Change actions to init on mount rather than hydrate #1653

Merged
merged 1 commit into from
Aug 23, 2018

Conversation

jacwright
Copy link
Contributor

Looking at the discussion on #1247 it sounds like this was the intended way actions would be set up to work (which is why we didn't add a mount lifecycle method). I believe this is a fix in the original implementation.

Complaints in chat about this surfaced the issue. Some libraries expect the element to be in the DOM when initializing and these libraries cannot be used without any lifecycle hook. @PaulMaly is requesting this be looked at, and I agree with his assesment.

What's more, this change should be backwards compatable. Actions which work before this change should continue working after this change.

Looking at the discussion on #1247 it sounds like this was the intended way actions would be set up to work (which is why we didn't add a `mount` lifecycle method). I *believe* this is a fix in the original implementation.

Complaints in chat about this surfaced the issue. Some libraries expect the element to be in the DOM when initializing and these libraries cannot be used without any lifecycle hook. @PaulMaly is requesting this be looked at, and I agree with his assesment.

What's more, this change *should* be backwards compatable. Actions which work before this change should continue working after this change.
@Rich-Harris Rich-Harris merged commit 7242905 into master Aug 23, 2018
@Rich-Harris Rich-Harris deleted the actions-onmount branch August 23, 2018 02:14
@Rich-Harris
Copy link
Member

nice, thanks 👍

@PaulMaly
Copy link
Contributor

@jacwright Seems, there some edge-cases when this still not working correctly. For example:

https://svelte.technology/repl?version=2.13.4&gist=45117e6b12980bdb59c2a5504aba649a

The problem here in <slot> element usage. Any ideas?

@jacwright
Copy link
Contributor Author

You could log this as a bug against slots. It appears that the contents of the slot are being "mounted" before the slot container is mounted. This prevents any of the children from being in the DOM when mount is called, so there is no way for actions to do what they need to do within the DOM in this case.

@Rich-Harris any performance problems or other reasons why slots shouldn't finish mounting their fragment before mounting their children?

@PaulMaly
Copy link
Contributor

@jacwright I'm sure there're some reasons for this bug, but I believe slots shouldn't mutate behavior of injected components. It breaks their encapsulation.

So, if we able to use some component without slots, we definitely should be able to use the same component in the slot too. Right?

@PaulMaly
Copy link
Contributor

@Rich-Harris Any thoughts? Very important bug, really!

@ekhaled
Copy link
Contributor

ekhaled commented Sep 26, 2018

@jacwright sometimes destroy() can be called on a component too quickly... before mount() is called.

At that point, it tries to call destroy on the action. But the action doesn't exist yet.
Leading to errors.

One way to fix would be:

block.builders.destroy.addLine(
-  `if (typeof ${name}.destroy === 'function') ${name}.destroy.call(#component);`
+  `if (${name} && typeof ${name}.destroy === 'function') ${name}.destroy.call(#component);`
);

I think that code has now moved to compile/render-dom/wrappers/Element/index.ts after @Rich-Harris 's overhaul

Does this seem like a reasonable fix?

ekhaled added a commit to ekhaled/svelte that referenced this pull request Sep 26, 2018
@ekhaled ekhaled mentioned this pull request Sep 26, 2018
Rich-Harris added a commit that referenced this pull request Oct 17, 2018
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.

4 participants