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

feat(desktop): add avro support #1757

Merged
merged 2 commits into from
Sep 14, 2024
Merged

feat(desktop): add avro support #1757

merged 2 commits into from
Sep 14, 2024

Conversation

LAST7
Copy link
Collaborator

@LAST7 LAST7 commented Sep 10, 2024

PR Checklist

If you have any questions, you can refer to the Contributing Guide

Does this PR introduce a breaking change?

  • Yes
  • No

Specific Instructions

Are there any specific instructions or things that should be known prior to review?

Other information

@LAST7 LAST7 self-assigned this Sep 10, 2024
@LAST7 LAST7 requested review from ysfscream and Red-Asuka and removed request for ysfscream September 10, 2024 11:15
@LAST7 LAST7 added feature This pr is a feature desktop MQTTX Desktop labels Sep 10, 2024
@ysfscream ysfscream added this to the v1.11.0 milestone Sep 11, 2024
@@ -0,0 +1,12 @@
export const convertObject = (raw: string, format?: PayloadType): string => {
Copy link
Member

Choose a reason for hiding this comment

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

There is a bit of confusion. Does the function of this utility function seem to be able to use the convertPayload function? 🤔

Copy link
Collaborator Author

@LAST7 LAST7 Sep 13, 2024

Choose a reason for hiding this comment

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

This is function originates from src/utils/protobuf.ts it should be used for the same logic in both avro and protobuf testing. Actually, I am also confused as it being a separate function from the convertPayload function.

It looks more similiar to the convertPayload in the cli part(cli/src/utils/convertPayload.ts) rather than the one in desktop part(src/utils/convertPayload.ts).

The logic of encoding/decoding message with format from these two components(cli & desktop) are quite different, take Base64 as example:

desktop:

const convertBase64 = (value: string, codeType: 'encode' | 'decode'): string => {
  const convertMap: CodeType = {
    encode(str: string): string {
      return btoa(unescape(encodeURIComponent(str)))
    },
    decode(str: string): string {
      return decodeURIComponent(escape(atob(str)))
    },
  }
  return convertMap[codeType](value)
}

cli:

const convertPayload = (payload: Buffer | string, format?: FormatType, action: Action = 'decode') => {
  const actions = {
    encode: {
      // It looks like its decoding from a base64 buffer
      base64: () => Buffer.from(payload.toString(), 'base64'),
      json: () => convertJSON(payload, 'encode'),
      ...

Actually, the methods inside cli's convertPayload look also utterly strange. Cuz to me, this base64: () => Buffer.from(payload.toString(), 'base64') seems to be decoding a base64 encoded message into a normal Buffer, which is strange as it's not performing any 'encode' action.

I think we should have an online discussion about these format conversion.

@ysfscream
Copy link
Member

There aren't any unit test cases at the moment. It's possible to run tests now. Could you consider adding more Avro tests to enhance and improve its stability?

@ysfscream
Copy link
Member

You can paste some usage screenshots in the comments to clarify the feature review, although we can check it locally. I think there is no big problem on my side with these code changes. However, I think I need to do more local testing after your PR has been merged.

LGTM. Thank you!

}
return convertedPayload
} catch (error) {
this.$message.error((error as Error).toString())
Copy link
Member

Choose a reason for hiding this comment

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

Although it might not significantly impact the program, for error handling in try-catch, you could refer to this solution on StackOverflow: https://stackoverflow.com/a/64452744/18652097.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a good idea to avoid returning undefined with such approach. But it seems that I would have to change the return type of the function to any or unknown to allow this object-like thing to be returned, which makes it nasty when validating the return variable of this function in the upper process.
Can you provide more detail on what return type should be stated and how to validate this { message: ... }? Thank you so much!

Copy link
Member

Choose a reason for hiding this comment

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

Specific to this example, I don't think you need to make any modifications, toString can be applied to the vast majority of data types.

Copy link
Collaborator Author

@LAST7 LAST7 Sep 13, 2024

Choose a reason for hiding this comment

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

Ah, you mean the error as Error thing.
I do agree that it's better to state it in the catch (error instanceof Error), but in most cases the project is using (error as Error).message or (error as Error).toString(), so I think following the same pattern would be a better choice.
After all, there's no significant difference between these two code style. :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, for this kind of simple error handling scenario, using type assertion is indeed more convenient. I just want to remind that this type of error handling may sometimes have hidden risks.

The try...catch block can catch errors of any type, not just objects that inherit from Error. For example, a piece of code may throw a string, a number, or even an object, which do not necessarily inherit from the Error class.

By using type assertion like error as Error, you may lose some actual information about the error. If the caught exception is not an instance of Error, this type assertion will result in missing important information in your subsequent handling, or even runtime errors.

TypeScript is designed to avoid unnecessary type assertions because excessive use of type assertions can make the code more fragile and harder to maintain. By leveraging TypeScript's type inference and type checking features as much as possible, such issues can be reduced.

Here I can give a relatively complex example, such as error handling for API calls, where we can encapsulate a generic handling method like this:

/**
 * Handles fetch errors.
 *
 * @param {unknown} error - The caught error object.
 * @param {() => boolean} [cb] - Optional callback function. If the callback exists and returns true, the default error handling will not be executed.
 */
export function handleFetchError(error: unknown, cb?: () => boolean) {
  if (error instanceof FetchError) {
    if (cb && cb())
      return

    const err = error as ResError
    const { statusText } = err.response || {}
    const { detail } = err?.response?._data || {}
    if (detail ?? statusText)
      ElMessage.error(detail ?? statusText)
    else
      console.error('Fetch error: ', error)
  }
  else {
    console.error('Unknown error: ', error)
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. Now I understand the hidden risk of using type assertion here, thank you for the explanation!
We could improve all the try-catch blocks in the project later for better error handling and safety.

@LAST7
Copy link
Collaborator Author

LAST7 commented Sep 13, 2024

You can paste some usage screenshots in the comments to clarify the feature review, although we can check it locally. I think there is no big problem on my side with these code changes. However, I think I need to do more local testing after your PR has been merged.

LGTM. Thank you!

Here's a usage video:

2024-09-13.17-24-13.mp4

Feel free to reply to this comment if there's anything that the showcase video does not cover.

1. expand the size of textboxes in Script -> Schema tab
2. use `$tc` instead of `$t` (i18n) to avoid type assertion
3. improve code style
@LAST7
Copy link
Collaborator Author

LAST7 commented Sep 14, 2024

The size of input/output textboxes in Script -> Schema tab are expanded:

Shot-2024-09-14-112224

@ysfscream ysfscream merged commit 287be0b into emqx:main Sep 14, 2024
2 checks passed
@LAST7 LAST7 deleted the avro branch September 17, 2024 07:09
@LAST7 LAST7 restored the avro branch September 17, 2024 07:09
@LAST7 LAST7 deleted the avro branch September 17, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop MQTTX Desktop feature This pr is a feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants