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

Using KustomExport types from external JS back in Kotlin #33

Open
shama opened this issue Jan 19, 2023 · 10 comments
Open

Using KustomExport types from external JS back in Kotlin #33

shama opened this issue Jan 19, 2023 · 10 comments

Comments

@shama
Copy link
Contributor

shama commented Jan 19, 2023

Hi! First, just wanted to say thanks for this library. It's made the TypeScript from Kotlin so much nicer to use. :)

I ran into an issue while using with external and have been looking into a fix but I'm fairly new to Kotlin so I could just be doing something wrong. I'm using the latest on the master branch as well as tried with 0.8.0 of KustomExport.

If I have a type exported from Kotlin with @KustomExport (e.g. class Item), create that type in JS (e.g. new Item()), and then send it back through Kotlin, accessing the properties from that object returns undefined.

I have this example Kotlin:

@KustomExport
class Item(
    val id: Long,
    val name: String
)

@KustomExport
class KotlinLibrary(private val js: JsLibrary) {
    fun getItem(): Item {
        return js.getItem()
    }
}

external class JsLibrary {
    fun getItem(): Item = definedExternally
}

and this JS:

export default class JsLibrary {
    getItem(): Item {
        return new Item(1, "Test")
    }
}
// ---
const js = new JsLibrary()
const kt = new KotlinLibrary(js)
const item = kt.getItem()
console.log(item.name) // undefined but should be "Test"

I've been attempting to add support for this use case but thought I'd open an issue just in case you are already familiar with this use case or know a way to support it. I've written a test for the use case here. The first four assertions pass but the others that interface with KustomExport do not pass.

It appears that returning an instance from JS, when it's ran through CommonItem.exportItem(), the getters that proxy to the internal common are no longer connected. I'm new to Kotlin though so that could be incorrect.

Thank you for your time!

@shama
Copy link
Contributor Author

shama commented Feb 1, 2023

Did more digging, I think the issue is because the type from the external isn't the Kotlin type but actually the generated .js type:

package thing

@KustomExport
class Item(
    val id: Long,
    val name: String
)

external class JsLibrary {
    fun getItem(): Item = definedExternally // This is actually type: thing.js.Item and not thing.Item
}

@KustomExport
class KotlinLibrary(private val js: JsLibrary) {
    fun getItem(): Item {
        // When getItem() is called, it should not be wrapped in an exportIem(result)
        // as it's already the expected JS Item
        return js.getItem()
    }
}

So now I just need to figure out a solution for that.

@glureau
Copy link
Collaborator

glureau commented Feb 1, 2023

Hi! Sorry I didn't found time to investigate that yet. You're probably right as we never tested with external/definedExternally.

From what I see the class Item is used from Js, so there is a generated wrapper around Item, and I would expect the JsLibrary to create the wrapper (thing.js.Item, that technically have an instance of thing.Item)... Am I right?

I need to think a bit more about the implications (and not a lot of time atm), but please share your ideas on this topic (and if you've the js/ts code or a github repository with this example it'll help me).


First, just wanted to say thanks for this library. It's made the TypeScript from Kotlin so much nicer to use. :)

Thanks, really appreciated.

@shama
Copy link
Contributor Author

shama commented Feb 1, 2023

Correct. The wrapper thing.js.Item has an instance of thing.Item. But I think because the internal class it's wrapping is not exposed, the names are mangled:
Screenshot 2023-02-01 at 10 53 23 AM

The CommonItem.exportItem(): Item = Item(this) thinks it's creating the internal thing.Item, so when initializing the properties of the internal common it's expecting the mangled property names. When coming from an external class, the this.common = common it sets in the internal constructor() is the wrapper class and not the internal common. So next time attempting to get a property from the object it's undefined.

I wrote a failing test case here: shama@082173c and I'm trying to figure out a solution. I'm still pretty new to Kotlin though.

One idea, we can try modifying fun CommonItem.exportItem() : Item = Item(this) to detect if this is already an Item and return it instead of re-wrapping it or maybe do some detection in the internal constructor?

Or another idea I was going to explore, write a Transformer for external classes that transforms it to use the .js based types but I'm not sure if it's feasible yet or maybe it still might hand that type to exportItem() and not the internal common one.

@glureau
Copy link
Collaborator

glureau commented Feb 2, 2023

Did you try to define an interface with the annotation instead of the external declaration?

I presume it works quite similarly, so we could generate from an (annotated) external declaration the same wrapper than for an interface, then use it when we receive the js implementation to unwrap the result.

@shama
Copy link
Contributor Author

shama commented Feb 7, 2023

That is a good idea. I tried with converting the externals to be interfaces and that fixes some instances where it knows to unwrap such as this:

fun getItemAt(at: Int): Item {
    return proxy.getItemAt(at)
}
// becomes
public fun getItemAt(at: Int): Item {
    val result = common.getItemAt(
        at = at,
    )
    return result.exportItem()
}

But if we tried to do something with the list of items, it doesn't know to unwrap and tries to use the JS provided items:

fun sumAmounts(): Long {
    return proxy.items.asList().fold(0) { sum, item -> sum + item.amount }
}
// becomes
public fun sumAmounts(): Double {
    // sumAmounts() will iterate over the List<Item> but they will be the JS provided Item
    val result = common.sumAmounts()
    return result.toDouble()
}

I tried to trick it into unwrapping the items first before using them:

fun getItems(): List<Item> {
    return proxy.items.asList()
}
// and then
fun getItems(): List<Item> {
    val res = mutableListOf<Item>()
    for ((i, item) in proxy.items) {
        res.add(getItemAt(i.toInt()))
    }
    return res
}

fun sumAmounts(): Long {
    return getItems().fold(0) { sum, item -> sum + item.amount }
}

But that didn't work as it's still calling common.sumAmounts() which operates on proxy.items thinking it's the common but it's actually the JS array of items. But maybe there is another way to tell it to unwrap the list of Items before using. Or maybe add a method to explicitly tell it to unwrap when mapping JS to Kotlin types.


Would you be open to adding some logic to the wrappers that detects if it's a common vs wrapper class? I can try working on a PR if I can get it working and propose a solution.

@glureau
Copy link
Collaborator

glureau commented Feb 8, 2023

Have you tried to pass the proxy via a setter? I can't investigate more right now, but you should have a generated class wrapping the typescript implementation in your kotlin code (if you can breakpoint and check that). I presume the issue is here because you're able to inject the proxy without its wrapper directly in 'common/kotlin' classes.

Totally open to contribution!

Would you be open to adding some logic to the wrappers that detects if it's a common vs wrapper class?
If this logic cannot be resolved at build time (preferred approach for performance reasons) I presume it'll be required in the wrapper class indeed.

@shama
Copy link
Contributor Author

shama commented Feb 13, 2023

Got a chance to try that. Passing the proxy via a setter/fun didn't work but if pass the list from the proxy via a setter, it works with interfaces.

This works when Item is an interface:

class ExternalClass() {
    var jsItems: List<Item> = emptyList()
    fun provideJsItems(newItems: List<Item>) {
        jsItems = items
    }
}
const thirdPartyClass = new ThirdPartyClass(
    [
        new Item(1, "One", 42),
        new Item(2, "Two", 100),
    ]
)
const externalClass = new ExternalClass()
externalClass.jsItems = thirdPartyClass.items
// or
externalClass.provideJsItems(thirdPartyClass.items)

So thanks again! I think that might be the solution we'll use until I figure out a good solution for external.


Setting the proxy itself didn't work, has the same issue where common is trying to use items from the js wrapper:

class ExternalClass() {
    lateinit var proxy: IThirdPartyClass
    fun provideProxy(newProxy: IThirdPartyClass) {
        proxy = newProxy
    }
}
const externalClass = new ExternalClass()
externalClass.proxy = thirdPartyClass
// or
externalClass.provideProxy(thirdPartyClass)

I'll set aside some time in the next couple weeks to dig into more. I appreciate your help!

@shama
Copy link
Contributor Author

shama commented Feb 13, 2023

@glureau Oh by chance, would you be able to do a release of adc689c?

@glureau
Copy link
Collaborator

glureau commented Feb 14, 2023

I just released the 0.8.1 for you, please tell me if it's ok for your needs as I can't test it on my previous project anymore.

@shama
Copy link
Contributor Author

shama commented Feb 14, 2023

Fantastic, thank you! It's working well for me.

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

No branches or pull requests

2 participants