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

Refactor theme install action. #11949

Merged
merged 2 commits into from
Mar 14, 2017
Merged

Conversation

budzanowski
Copy link
Contributor

Info

Installing a theme that had a parent theme required also installation of that parent theme.
Code for that was working but was using promises in non optimal way.

Testing

Verify that on Jetpack site themes that have parent themes still install successfully.
Example themes with parents Goran( Edin ) or Sidekick( Superhero )

@budzanowski budzanowski added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Janitorial labels Mar 9, 2017
@budzanowski budzanowski self-assigned this Mar 9, 2017
@budzanowski budzanowski requested review from ockham and seear March 9, 2017 15:53
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] S Small sized issue label Mar 9, 2017
@ockham
Copy link
Contributor

ockham commented Mar 9, 2017

Thinking some more about this, are we actually guaranteeing that both the child and parent are installed before attempting to activate the child?

Parent theme install is only attempted after child install (ref), so that's fine. But is it possible that we should return that dispatch so that this .then() is only called afterwards? (I'm honestly not sure.)

@seear
Copy link
Contributor

seear commented Mar 9, 2017

But is it possible that we should return that dispatch so that this .then() is only called afterwards?

@ockham Yeah, I pondered this very question in the PR where we moved the parent install, and concluded that return would only make a difference to the control flow if there was something after the return statement in the block, which there isn't.

@seear
Copy link
Contributor

seear commented Mar 9, 2017

Yeah, I pondered this very question in the PR where we moved the parent install, and concluded that return would only make a difference to the control flow if there was something after the return statement in the block, which there isn't.

However :) , looking at the code again, I think my conclusion was wrong. We need to return the parent install promise, otherwise the activate code is only waiting on the first install.

@ockham
Copy link
Contributor

ockham commented Mar 9, 2017

However :) , looking at the code again, I think my conclusion was wrong. We need to return the parent install promise, otherwise the activate code is only waiting on the first install.

Yeah, looking at reduxjs/redux#1676 (comment), I was coming to the same conclusion 😄

@budzanowski Wanna add that return as part of this PR?

@budzanowski
Copy link
Contributor Author

I was thinking about it when working on this :) And concluded that we do not need this.
For activation to succeed we only need to have child theme installed. We need both parent and child so the child can actually work. But as it turned out child will be still active if parent is not present. Active but broken. The only real waiting required is between installs as it turned out ( from observation ) that having two installs too close to each other can cause the second install to fail with unknown reason. So having chain of actions install child -> activate child -> install parent is absolutely fine.

@matticbot matticbot added the [Size] S Small sized issue label Mar 13, 2017
@budzanowski budzanowski force-pushed the update/refactor-install-action branch from 974513f to 5e5aa79 Compare March 13, 2017 16:53
@matticbot matticbot added the [Size] S Small sized issue label Mar 13, 2017
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@budzanowski budzanowski merged commit 6648c23 into master Mar 14, 2017
@budzanowski budzanowski deleted the update/refactor-install-action branch March 14, 2017 13:22
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Appearance & Themes Features related to the appearance of sites. [Size] S Small sized issue [Type] Janitorial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants