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

Create AxiosSearch.kt #23

Merged
merged 9 commits into from
Nov 28, 2017
Merged

Create AxiosSearch.kt #23

merged 9 commits into from
Nov 28, 2017

Conversation

ScottHuangZL
Copy link
Contributor

An example to show how to leverage axios lib to fetch remote data by [email protected] ([email protected])
(Btw, Vue.js formally leverage axios for ajax related works, so it should be good if React.js also use it)

It will be helpful for people to use axios lib to make their static data web to live/real data web, thanks.

Date: Nov 25, 2017

/**
 * An example to show how to leverage axios lib to fetch remote data by [email protected] ([email protected])
 * (Btw, Vue.js formally leverage axios for ajax related works, so it should be good if React.js also use it)
 *
 * Date: Nov 25, 2017
 */

/**
* An example to show how to leverage axios lib to fetch remote data by [email protected] ([email protected])
* (Btw, Vue.js formally leverage axios for ajax related works, so it should be good if React.js also use it)
Copy link
Contributor

Choose a reason for hiding this comment

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

React is a rendering library not a framework, so working with network is out of its scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agree.
But better add axios example into kotlin-react for people to exchange remote data :)

// I personally download axios.min.js proactive in put it into public\static\js folder
// <script src="%PUBLIC_URL%/static/js/axios.min.js"></script>
// or you can put <script src="https://unpkg.com/axios/dist/axios.min.js"></script> into index.html to avoid proactive download to local
// And we provide a simple fun to wrap axios.js, it is not type safe, suggest JB team to provide a formal wrapping for this useful lib
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use commonjs instead:

@JsModule("axios")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your advise!

Change from
// You need input correct axios.min.js link address in public\static\index.html
// I personally download axios.min.js proactive in put it into public\static\js folder
// <script src="%PUBLIC_URL%/static/js/axios.min.js"></script>
// or you can put   <script src="https://unpkg.com/axios/dist/axios.min.js"></script>     into index.html to avoid proactive download to local
// And we provide a simple fun to wrap axios.js, it is not type safe, suggest JB team to provide a formal wrapping for this useful lib

to
@jsmodule("axios")
value = state.zipCode
placeholder = "Input zip code ..."
title = infoText
onChangeFunction = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's extract it into a component method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, please. You can direct change it. Thanks.

//Per Hypnosphi advice, change to common js way.
//you should need "npm install axios --save" in advance in your project folder
@JsModule("axios")
external fun axios(config: dynamic): dynamic
Copy link
Contributor

@Hypnosphi Hypnosphi Nov 25, 2017

Choose a reason for hiding this comment

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

We can add some basic typing right in the example:

external interface AxiosConfig {
  var url: String
  var timeout: Number
}

// T is the type of response data
external interface AxiosResponse<T> {
  data: T
}

@JsModule("axios")
external fun axios<T>(config: AxiosConfig): Promise<T>

Refactor onChangeFunction to 2 small fun.
Add simple type for the axios.

I am not familiar with how to add type for external javascript lib.  Please feel free to upgrade/enhance from my code directly. Thanks.
zipResult = ZipResult("", "", "")
}

private fun remoteSearchZip(zipCode: String) {
val config: AxiosConfigSettings = js {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can now use jsObject, which is type-safe

In fact, js is just a shortcut for jsObject<dynamic>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the great advice. Have change in code:)

change js{} to jsObject{} for type safe
@Hypnosphi
Copy link
Contributor

Would you like to add typings for response data as well?

Add typing both for axios response and error
@ScottHuangZL
Copy link
Contributor Author

Done to add typing both for axios response and error too.

After your review/comment, I get new skills:)

Add more typing for axios config/response/error
Now, people should be able leverage typed axios lib to more easily  co-work with Kotlin-React to create a real app.
setState {
zipResult = ZipResult("", "", "Find error, please open your console to learn detail.")
zipResult = ZipResult("Find error:", error.message, ". Please open your console to learn detail")
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add an errorMessage field to state instead of misusing ZipResult data class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will do.

}

external interface AxiosResponse {
var data: dynamic
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make axios return Promise<AxiosResponse> instead of `dynamic.

We can also make both AxiosResponse and axios a generic type so that data has the type of type parameter not dynamic:

external interface AxiosResponse<T> {
  // we can use `val` instead of `var` here because we never assign those values ourselves
  val data: T
  ...
}

@JsModule("axios")
external fun <T> axios(config: AxiosConfigSettings): Promise<AxiosResponse<T>>

Then you can add type for data:

external interface Data {
  val country: String
  val state: String,
  val city: String
}

and use axios like this:

// the type for `response` should be inferred authomatically
axios<Data>(config).then { response -> ... }.catch { error: AxiosError -> ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is a good idea to add more typing due to below reason:

  1. The axios not only get response, but also have catch error branch.
    So, external fun axios(config: AxiosConfigSettings): Promise<AxiosResponse> not correct.

  2. When I try hide/ignore catch branch firstly. The compile show below error:
    How to resolve it?
    Can you show example code? Thanks.

src\App\AxiosSearch\AxiosSearch.kt:92:29: error: no value passed for parameter 'onFulfilled'
axios(config).then { response ->
^
src\App\AxiosSearch\AxiosSearch.kt:94:48: error: unresolved reference: data
zipResult = ZipResult(response.data.country, response.data.state, response.data.city)
^
src\App\AxiosSearch\AxiosSearch.kt:94:71: error: unresolved reference: data
zipResult = ZipResult(response.data.country, response.data.state, response.data.city)
^
src\App\AxiosSearch\AxiosSearch.kt:94:92: error: unresolved reference: data
zipResult = ZipResult(response.data.country, response.data.state, response.data.city)
^
src\App\AxiosSearch\AxiosSearch.kt:96:34: error: unresolved reference: status
console.log(response.status)
^
src\App\AxiosSearch\AxiosSearch.kt:97:34: error: unresolved reference: statusText
console.log(response.statusText)

  1. When I add back catch branch, it show below error:
    src\App\AxiosSearch\AxiosSearch.kt:102:18: error: type inference failed: fun catch(onRejected: (Throwable) -> S): Promise
    cannot be applied to
    ((AxiosError) -> Unit)

             .catch { error: AxiosError ->
              ^
    

src\App\AxiosSearch\AxiosSearch.kt:102:24: error: type mismatch: inferred type is (AxiosError) -> Unit but (Throwable) -> Unit was expected
.catch { error: AxiosError ->
^
src\App\AxiosSearch\AxiosSearch.kt:102:26: error: expected parameter of type Throwable
.catch { error: AxiosError ->

Copy link
Contributor

@Hypnosphi Hypnosphi Nov 27, 2017

Choose a reason for hiding this comment

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

First, you have to put the { response -> ... } lambda in parens, because it's not actually the last argument of then:

axios<Data>(config).then({ response ->
  setState {
    zipResult = ZipResult(response.data.country, response.data.state, response.data.city)
    errorMessage = ""
  }
})

Second, the Promise builtin type in Kotlin expects the failure argument to be a Throwable. For this example it's OK, because we only use the message field:

.catch { error ->
  setState {
    zipResult = ZipResult("", "", "")
    // the Elvis operator is needed because `Throwable::message` is nullable
    errorMessage = error.message ?: ""
  }
}

Add response data type
add errorMessage field
It finally works for geeric type:)
Thanks for @Hypnosphi professional advice, does learn something from him!
@ScottHuangZL
Copy link
Contributor Author

Add generic type for Axios, it finally works!
Thanks for @Hypnosphi 's professional advice, does learn something from this practice:)

@ScottHuangZL
Copy link
Contributor Author

Thanks for approve the changes:) Will will merge into branch? Thanks.

@Hypnosphi
Copy link
Contributor

Thanks @ScottHuangZL!

@Hypnosphi Hypnosphi merged commit 5912272 into JetBrains:master Nov 28, 2017
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