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

Feature Vaccine #83

Merged
merged 7 commits into from
Nov 23, 2018
Merged

Feature Vaccine #83

merged 7 commits into from
Nov 23, 2018

Conversation

zenangst
Copy link
Collaborator

I have this idea of deprecating Vaccine as a framework and ship the implementation together with Injection. Vaccine will be an opt-in feature that will be enabled and disabled in the same way as you enable TDD.

If you have the implementation enabled and your class does not respond to injected, it will fall back to running Vaccine in order to rebuild the view controllers or views state.

I've tested this implementation on iOS and tvOS and it works as the Vaccine framework does.

The biggest change to the client-server implementation is that a message is sent to the client notifying it of the current vaccine preference setting.

Copy link
Owner

@johnno1962 johnno1962 left a comment

Choose a reason for hiding this comment

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

I have to admit I’m not very keen on the change to not check whether a class implements the method “injected()” before it is add of the list of classes to be “swept”. The Sweep is the least reliable part of injection and it’s best not to attempt it if the user doesn’t opt in by implementing “injected()” on a class being injected. Otherwise the overall reliability of injection will be reduced.

…s to `injected()`.

If Vaccine is enabled, it will be invoked before the `injected` method is called.
@zenangst
Copy link
Collaborator Author

@johnno1962 I see your point, thanks for the feedback mate! Was a bit unsure how important the sweeper was, thanks for clearing that up. So I pushed a new commit that restores the sanity check for the sweeper and now the injected method will be invoked after vaccine is done recreating the objects state, but only if vaccine is enabled. So the default behavior for InjectionIII remains the same.

@johnno1962
Copy link
Owner

johnno1962 commented Nov 23, 2018

Do you need to check if the instance implements injected() now?? Just a thought.

@zenangst
Copy link
Collaborator Author

@johnno1962 nice catch, that extra responds(to:) has been removed :)

@johnno1962
Copy link
Owner

johnno1962 commented Nov 23, 2018

Looks good to me now. Don’t forget to squash 👍

@zenangst zenangst merged commit 64f2a32 into master Nov 23, 2018
@zenangst zenangst deleted the feature/vaccine branch November 23, 2018 09:06
@zenangst
Copy link
Collaborator Author

Squash is the new default!

@zenangst
Copy link
Collaborator Author

@johnno1962 I really appreciate you taking the time to review my PR's! ❤️

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.

2 participants