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

[WIP] proof of concept for TraitModifiable generics #8

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

antonselyanin
Copy link

Issue #7

Work in progress, this is just a 'proof of concept' for the idea (all tests are passing, live reloading works).

One question before I can move forward :).

Do you want to keep the original type checking method (list of restricted classes) or we can substitute it with generics? TypedTrait is a temporary thing.

@krzysztofzablocki
Copy link
Owner

so the list of restricted classes was there just so we can have multiple different types while preventing mistakes, this approach is better if live reloading, and code injection still work 👍

@antonselyanin
Copy link
Author

(finally, I have some free time :))

The idea is falling apart :D

  1. It is impossible to write and inject a new TypedTrait into a running app, at least if the generic type is a protocol.

AFAIK, Swift protocols require static linking. IMO, this is a minor drawback, I personally wouldn't mind restarting an app every time I had to implement a new Trait. If you do TDD, it's inevitable ;)

  1. It turns out objc_getClassList function might not return all classes that exist in the app.

I guess, the runtime doesn't "load" all the classes immediately after start.
In this PR DropShadowSpec fails in "serializes correctly with JSON" test because DropShadow class isn't beeing registered on startup.
Adding this code Trait.factories = Trait.getTraitFactories() before the test fixes the issue, which confirms the problem.

It looks like it occurs arbitrarily, in my case it breaks only when I convert DropShadow to TypedTrait.
I think auto-registration via objc_getClassList is unreliable :(
What are other options? Manual Trait registration?

  1. API design, {Trait}Modifiable protocols should be well thought out

When I converted Font to TypedTrait, I just copied attributes from UILabel and I got this ugly protocol:

public protocol FontModifiable: class {
    var font: UIFont! { get set }
    var textColor: UIColor! { get set }
}

Implicitly unwrapped optionals are ugly, textColor looks alien here.

@krzysztofzablocki
Copy link
Owner

So I couldn't remember why I didn't go with the protocol idea from the start, but I remember thinking it would provide issues when you want to leverage runtime, which is confirmed by your research here

  1. This is not such a big deal for production code, but its nice to be able to experiment with Traits before they are finalised.

  2. Runtime should be reliable for regular objc classes like in the existing architecture, I used this pattern back in objc days for a self resolving architecture and it worked well for years, blog post about it from 2014

I think it stops being reliable when we start incorporating Swift specific features like the generic protocol approach.

  1. Definitely, I think those have to be wrapped in different functions, rather than trying to use same API as UIKit provides, as there might be differences across specific instances of UIKit and we'd want to avoid name conflicts.
public protocol FontModifiable: class {
    var modifableFont: UIFont? { get set }
}

And just extending each type with conformance to the protocol and using computed variable that forwards to the corresponding methods.

I think that using protocols is much nicer / safer, but with loosing auto-registration and injection together I wonder if its worth it, what are your thoughts here?

@antonselyanin
Copy link
Author

Auto-registration could be replaced with NSStringFromClass/NSClassFromString. It adds a "namespace" prefix, so Trait classes from the library are serialized into JSON as "Traits.Font", "Traits.BackgroundColor" and so on.

I think it's rather an advantage. If someone wanted to have their own implementation of Font trait they wouldn't have to give it a unique name. Like, you could have a library "BetterTraits" with it's own Font, DropShadow etc.

What do you think?

@krzysztofzablocki
Copy link
Owner

good idea 👍

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