Brightify / ReactantUI

Reactant extension for UI declaration in XML
MIT License
19 stars 2 forks source link

Move generated code into templates #32

Open josefdolezal opened 6 years ago

josefdolezal commented 6 years ago

Hey @brightify! Thanks for this amazing library and the hard work you made on it! I would like to propose refactoring of Generator classes.

Context

The current state and approach of ReactantUI is to generate Swift code using Generator.swift, StyleGenerator.swift and partialy main.swift. This makes the mentioned files big and hard to read. Also none of these are tested.

Proposal

Separate the logic from string based templates (Generator.swift, StyleGenerator.swift, main.swift) using template files. This would require us to add templating language dependency. There are multiple Swift templating languages, for example Stencil, leaf and GRMustache.swift which all looks good.

After we select the templating language, we can move all the string templates into appropriate template files. It will make lightweight Generator classes which will be easier to read. Once the Generators will not contain the formatting logic, we will be able to introduce unit tests! 🎉 To keep the templates as dummy as we can, it would be nice to introduce simple DSL for that. As this is just fecatoring, the public API will remain the same.

I tried to quickly make a proof of concept branch, but the migration is more chore than I initially though.

Note: This is just an proposal, we can choose different approach or completely close the issue - not pressure on this! The main of this proposal is to:

Thanks for your time, would love to hear a feedback!

TadeasKriz commented 6 years ago

Thank you for this great proposal!

We switched to Stencil a long time ago in Cuckoo and it has a lot of advantages, but also some disadvantages. I'd like to look into the multiline literal String support before switching to a template library. Would that make sense from what you're seeing?

I agree that the Generator code is a mess now, it was written in a hurry and is in dire need of refactoring. I just want to make sure we select the best one before committing to it (as you said, it takes a LOT of time to switch to templates).

TadeasKriz commented 6 years ago

I just remembered, we need to add SourceKitten to parse Swift files to have more context about the code. That way we'll be able to add more compile-time checks for the XML files.

Testing the Generator is a tough thing to do. We did it in Cuckoo, but it was a pain to keep updated. I propose we write UI tests and test the generator indirectly, by ensuring the UI looks (Snapshot tests) and works as expected.

josefdolezal commented 6 years ago

Thanks for quick feedback!

I agree that moving to templates will definitely introduces some gotchas, it's always about compromises. 😒

I am not sure about default support multiline strings, but we can create custom filters which will handle it.

The SourceKitten is must have, great idea! It should be different separate feature though.

About testing, since this tool is still pretty small, we can omit them now. If we decide to use templates, we can add tests after the migration is done.

If you agree, I can do some basic implementation within few days. We can decide about the approach then.

TadeasKriz commented 6 years ago

What I meant with multiline strings is that we could still generate the code from Swift itself, just use multiline literals instead of the way it's done now. For example this:

if imports {
            l("import UIKit")
            l("import Reactant")
            l("import SnapKit")
            if isLiveEnabled {
                l("#if (arch(i386) || arch(x86_64)) && os(iOS)")
                l("import ReactantLiveUI")
                l("#endif")
            }
}

would get changed to this:

if imports {
            l("""
                import UIKit
                import Reactant
                import SnapKit
            """)
            if isLiveEnabled {
                l("""
                    #if (arch(i386) || arch(x86_64)) && os(iOS)
                    import ReactantLiveUI
                    #endif
                """)
            }
}