nwillc / ksvg

A Multiplatform Kotlin SVG image DSL.
ISC License
81 stars 16 forks source link

Request for multiplatform #3

Open NikkyAI opened 4 years ago

NikkyAI commented 4 years ago

Is it possible to package this for multiplatform?

for my own project i just copied your code into my commonMain

i use this with kotlin/js and react, but having it in common code leaves the option open to render stuff serverside too

seems like Chat.isUpperCase() does not exist on js so i solved it expect/actual, iirc this is the only problem i ran into

//common
expect val Char.isUpperCase: Boolean

// js
actual val Char.isUpperCase: Boolean
    get() = (this == this.toUpperCase() && this != this.toLowerCase())

// jvm
actual val Char.isUpperCase: Boolean
    get() = isUpperCase()
nwillc commented 4 years ago

I've thought this would be a good thing to do. I've not done any multi platform yet. Will look at it.

On Sun, Jan 12, 2020 at 4:01 AM NikkyAI notifications@github.com wrote:

Is it possible to package this for multiplatform?

for my own project i just copied your code into my commonMain

i use this with kotlin/js and react, but having it in common code leaves the option open to render stuff serverside too

seems like Chat.isUpperCase() does not exist on js so i solved it expect/actual, iirc this is the only problem i ran into

//common expect val Char.isUpperCase: Boolean

// js actual val Char.isUpperCase: Boolean get() = (this == this.toUpperCase() && this != this.toLowerCase())

// jvm actual val Char.isUpperCase: Boolean get() = isUpperCase()

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nwillc/ksvg/issues/3?email_source=notifications&email_token=AA3VMXM7WEPCA6Q52JNPIHLQ5LL5VA5CNFSM4KFWQ7Q2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IFSCURA, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3VMXJNNMPTSXVNUF3EYMLQ5LL5VANCNFSM4KFWQ7QQ .

nwillc commented 4 years ago

I assume the logging support will need fixing too.

NikkyAI commented 4 years ago

i would suggest kotlin-logging, not that there is much to replace.. have a look at my port: https://github.com/NikkyAI/pentagame/tree/a0311c68ade0dc768a539c6e8da6a8901682aa92/shared/src/commonMain/kotlin/com/github/nwillc/ksvg and the Character.isUpperCase code here https://github.com/NikkyAI/pentagame/blob/ca4215cdade3e9f8b3be5519365620952dd4c9bd/shared/src/jsMain/kotlin/isUpperCase.kt

nwillc commented 4 years ago

I've undertaken making the project multi-platform. Won't happen instantly, and happy for any help, but I am working on it in my free time.

On Mon, Jan 13, 2020 at 1:30 AM NikkyAI notifications@github.com wrote:

i would suggest kotlin-logging, not that there is much to replace.. have a look at my port: https://github.com/NikkyAI/pentagame/tree/a0311c68ade0dc768a539c6e8da6a8901682aa92/shared/src/commonMain/kotlin/com/github/nwillc/ksvg and the Character.isUpperCase code here https://github.com/NikkyAI/pentagame/blob/ca4215cdade3e9f8b3be5519365620952dd4c9bd/shared/src/jsMain/kotlin/isUpperCase.kt

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nwillc/ksvg/issues/3?email_source=notifications&email_token=AA3VMXID7NS5DGFOQEE5GDDQ5QDAPA5CNFSM4KFWQ7Q2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIXU67Y#issuecomment-573525887, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3VMXKHANONDBAOMLRG72DQ5QDAPANCNFSM4KFWQ7QQ .

nwillc commented 4 years ago

I may have a working version. Would you be able to look at the feature/multi-platform branch to review the JS work? I don't have expertise to evaluate the JS bits.

On Mon, Jan 13, 2020 at 9:42 PM nwillc@gmail.com wrote:

I've undertaken making the project multi-platform. Won't happen instantly, and happy for any help, but I am working on it in my free time.

On Mon, Jan 13, 2020 at 1:30 AM NikkyAI notifications@github.com wrote:

i would suggest kotlin-logging, not that there is much to replace.. have a look at my port: https://github.com/NikkyAI/pentagame/tree/a0311c68ade0dc768a539c6e8da6a8901682aa92/shared/src/commonMain/kotlin/com/github/nwillc/ksvg and the Character.isUpperCase code here https://github.com/NikkyAI/pentagame/blob/ca4215cdade3e9f8b3be5519365620952dd4c9bd/shared/src/jsMain/kotlin/isUpperCase.kt

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nwillc/ksvg/issues/3?email_source=notifications&email_token=AA3VMXID7NS5DGFOQEE5GDDQ5QDAPA5CNFSM4KFWQ7Q2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIXU67Y#issuecomment-573525887, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3VMXKHANONDBAOMLRG72DQ5QDAPANCNFSM4KFWQ7QQ .

NikkyAI commented 4 years ago

i assume with js specific code you mean the logging?

it looks like it will work but i think it will not show the original file location / line numbers when using the browser dev tools for that iirc the logger calls need to all be inline or so, never looked into it in detail korlibs/klogger works for me, but it has no decent formatting

also 1.3.70-eap had some mindblowing improvements to the js side of things although it is fine for this, since you are depending on barely anything

through experimentation i found out that this or something similar is required in the buildscript

js() {
        useCommonJs()
        browser {
            runTask {
                sourceMaps = true
            }
            webpackTask {
                sourceMaps = true
            }
        }
        compilations.all {
            kotlinOptions {
                sourceMap = true
                metaInfo = true
                main = "call"
            }
        }
        mavenPublication { // Setup the publication for the target
//            artifactId = "ksvg-js"
            // Add a docs JAR artifact (it should be a custom task):
//            artifact(javadocJar)
        }
    }

without it the project using it will complain that ksvg is not configured for js this error only nees to pop up when it is included as subproject, but after fixing it using it as a library from mavenLocal seems to work too

nwillc commented 4 years ago

I've accepted your PR. It's not the logging I'm worried about. I don't have a JS test harness to try things with and am struggling getting JS unit testing going.

NikkyAI commented 4 years ago

i can look into that

also have similar problems of not knowing how to run tests in the browser

NikkyAI commented 4 years ago

seems like this is breaking things.. https://stackoverflow.com/questions/51475050/spaces-in-test-method-names-when-targeting-multiplatform

TL;DR: cannot have spaces in testnames in commonTest because it cannot compile to js

NikkyAI commented 4 years ago

after fixing all the testnames with @JsName

these errors are left over

here jvm and js seem to sort x1, x2, y1, y2 differently https://github.com/nwillc/ksvg/blob/72402ec00c1f4c33ec4f2d5f01335ef12000d207/src/commonTest/kotlin/com/github/nwillc/ksvg/elements/LINETest.kt#L48

IllegalArgumentException: Value (10) is not a valid length or percentage https://github.com/nwillc/ksvg/blob/72402ec00c1f4c33ec4f2d5f01335ef12000d207/src/commonTest/kotlin/com/github/nwillc/ksvg/elements/RECTTest.kt#L43

IllegalArgumentException: Value (M 10,30 A 20,20 0,0,1 50,30 Z) is not a valid path https://github.com/nwillc/ksvg/blob/72402ec00c1f4c33ec4f2d5f01335ef12000d207/src/commonTest/kotlin/com/github/nwillc/ksvg/elements/PATHTest.kt#L28

in addition to that there seem to be a handful failures that look like similar regex or sorting failures https://github.com/nwillc/ksvg/blob/c511c26534111ea9882eefea4256194ef12b7f0e/src/commonTest/kotlin/com/github/nwillc/ksvg/attributes/AttributeTypeTest.kt

for the last few errors it seems like the regex might be acting up, i will open a PR again for the current progress

nwillc commented 4 years ago

The issues in the tests I believe are resolved. The sort order is now enforced and the other bits and pieces seem resolved. The JVM build artifacts now work with existing clients so I've merged the branch to master. I think the javascript is working but I still don't understand the packaging and how it would be used in an example. More to do there.

harryjph commented 2 years ago

Please see #10 for instructions on how to use this in a multiplatform project