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

JavaScript #3208

Merged
merged 65 commits into from
Oct 15, 2021
Merged

JavaScript #3208

merged 65 commits into from
Oct 15, 2021

Conversation

Pitel
Copy link
Contributor

@Pitel Pitel commented Jul 2, 2021

Apollo support for Kotlin/JS

⚠️ Currently in heavy WIP state, consulting with @martinbonnin on Slack.

@apollo-cla
Copy link

@Pitel: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@Pitel
Copy link
Contributor Author

Pitel commented Jul 9, 2021

@martinbonnin

e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/fullstack/com/apollographql/apollo3/integration/fullstack/BookTripMutation.kt: (22, 3): JavaScript name (id) generated for this declaration clashes with another declaration: fun id(): String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/fullstack/com/apollographql/apollo3/integration/fullstack/BookTripMutation.kt: (24, 3): JavaScript name (id) generated for this declaration clashes with another declaration: val id: String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/fullstack/com/apollographql/apollo3/integration/fullstack/CancelTripMutation.kt: (22, 3): JavaScript name (id) generated for this declaration clashes with another declaration: fun id(): String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/fullstack/com/apollographql/apollo3/integration/fullstack/CancelTripMutation.kt: (24, 3): JavaScript name (id) generated for this declaration clashes with another declaration: val id: String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/fullstack/com/apollographql/apollo3/integration/fullstack/LaunchDetailsQuery.kt: (22, 3): JavaScript name (id) generated for this declaration clashes with another declaration: fun id(): String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/fullstack/com/apollographql/apollo3/integration/fullstack/LaunchDetailsQuery.kt: (24, 3): JavaScript name (id) generated for this declaration clashes with another declaration: val id: String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/normalizer/com/apollographql/apollo3/integration/normalizer/CharacterDetailsQuery.kt: (23, 3): JavaScript name (id) generated for this declaration clashes with another declaration: fun id(): String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/normalizer/com/apollographql/apollo3/integration/normalizer/CharacterDetailsQuery.kt: (25, 3): JavaScript name (id) generated for this declaration clashes with another declaration: val id: String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/normalizer/com/apollographql/apollo3/integration/normalizer/CharacterNameByIdQuery.kt: (21, 3): JavaScript name (id) generated for this declaration clashes with another declaration: fun id(): String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/normalizer/com/apollographql/apollo3/integration/normalizer/CharacterNameByIdQuery.kt: (23, 3): JavaScript name (id) generated for this declaration clashes with another declaration: val id: String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/normalizer/com/apollographql/apollo3/integration/normalizer/CharacterWithBirthDateQuery.kt: (22, 3): JavaScript name (id) generated for this declaration clashes with another declaration: fun id(): String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/normalizer/com/apollographql/apollo3/integration/normalizer/CharacterWithBirthDateQuery.kt: (24, 3): JavaScript name (id) generated for this declaration clashes with another declaration: val id: String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/normalizer/com/apollographql/apollo3/integration/normalizer/StarshipByIdQuery.kt: (22, 3): JavaScript name (id) generated for this declaration clashes with another declaration: fun id(): String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/normalizer/com/apollographql/apollo3/integration/normalizer/StarshipByIdQuery.kt: (24, 3): JavaScript name (id) generated for this declaration clashes with another declaration: val id: String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/normalizer/com/apollographql/apollo3/integration/normalizer/UpdateReviewMutation.kt: (23, 3): JavaScript name (id) generated for this declaration clashes with another declaration: fun id(): String
e: /home/pitel/apollo-android/composite/tests/integration-tests-kotlin/build/generated/source/apollo/normalizer/com/apollographql/apollo3/integration/normalizer/UpdateReviewMutation.kt: (26, 3): JavaScript name (id) generated for this declaration clashes with another declaration: val id: String

It look like you'll have to change someting in generating code, because the id is clashing.

@martinbonnin
Copy link
Contributor

martinbonnin commented Jul 9, 2021

Mmmm I don't think this changed recently but this is going to be a problem nonetheless... The problem can be reduced to:

class MyOperation(val id: String) {
  fun id(): String {
    return ""
  }
}

My understanding is that JS classes cannot have properties with the same name as a function?

If that's the case, we can decrease the occurence of name clashes with @JsName:

class MyOperation(val id: String) {
  @JsName("operationId")
  fun id(): String {
    return ""
  }
}

To 100% remove nameclashes, it would then either be:

  1. using aliases in queries
  2. or escape property names in the codegen

I think we should do 2. I'll take a look at this.

@Pitel
Copy link
Contributor Author

Pitel commented Jul 19, 2021

@martinbonnin Do you have some ETA on the fix?

@martinbonnin
Copy link
Contributor

martinbonnin commented Jul 19, 2021

@Pitel pretty hard to tell at the moment but I think you can workaround by changing the name of Operation.id in Js:

  /**
   * An unique identifier for the operation. 
   */
  @JsName("operationId")
  fun id(): String

That should do until we find a more robust way to escape name clashes.

@Pitel
Copy link
Contributor Author

Pitel commented Jul 19, 2021

@martinbonnin Thanks for the link. I didn't know where to look.

Now it builds, and the tests are failing, as expected.

I'll probably have to modify the existing tests, because there is no runBlocking in JS. So I'll probably do multiplatform runTest method (using runBlocking on JVM and Apple, and Promise on JS), which will have to wrap all tests that were using the runBlocking and remove the runBlocking, because I simply can't do that in JS.

@martinbonnin
Copy link
Contributor

We have runWithMainLoop that is doing some specific stuff for Apple so it might make sense to add Js support there as well (apollo-testing-support module) ?

build.gradle.kts Outdated Show resolved Hide resolved
Remove forgotten println
JS doesn't have threads anyway.
Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

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

Thanks for diving into this ! That's an awesome addition, will be very welcome 🎉 😃!

One last thing before I forget. Can you remind again why we need to target the IR compiler (and therefore okio 3.0) ? That's something we might need to fine tune if we go stable before okio 3.

@Pitel
Copy link
Contributor Author

Pitel commented Oct 12, 2021

@martinbonnin Well, maybe we don't exactly need it, but JetBrains is encouraging developers to start using it. And most KMP projects and libraries I've seen and used already does.

https://kotlinlang.org/docs/js-ir-compiler.html

Yup, the Okio ALPHA looks a bit scary, but I would say that if the tests passes, then it's probably fine. In the begining, we've hoped that Okio 3 will be stable when Apollo JS will be finished, but is still isn't.

I suppose the Okio 2 and 3 have the same API, so maybe it's possible to change the dependency just for JS builds (and "market" the JS as experimental or something)? 🤔

@martinbonnin
Copy link
Contributor

@martinbonnin Well, maybe we don't exactly need it, but JetBrains is encouraging developers to start using it. And most KMP projects and libraries I've seen and used already does.

Makes sense 👍 . Thanks for the explanation!

I suppose the Okio 2 and 3 have the same API, so maybe it's possible to change the dependency just for JS builds (and "market" the JS as experimental or something)?

Exactly. Experimental is fine for JS at this point. We just don't want to ship a stable JVM artifact that depends on an alpha of Okio. Follow up issue there: #3397

Copy link
Contributor

@BoD BoD left a comment

Choose a reason for hiding this comment

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

👏

@hwillson hwillson added this to the Release 3.0 beta01 milestone Oct 14, 2021
@martinbonnin martinbonnin merged commit be49770 into apollographql:dev-3.x Oct 15, 2021
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.

5 participants