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

runtime type consistency in expression evaluation. #221

Open
Yuyz0112 opened this issue Jan 20, 2022 · 10 comments
Open

runtime type consistency in expression evaluation. #221

Yuyz0112 opened this issue Jan 20, 2022 · 10 comments
Assignees
Labels
refactor runtime about meta-ui runtime

Comments

@Yuyz0112
Copy link
Contributor

https://github.com/webzard-io/sunmao-ui/blob/5fe5163733/packages/runtime/src/services/stateStore.ts#L106-L108

Currently, we handle expression evaluation errors with error logs but will return undefined as the result.

This makes components can not predict the types of props in runtime. For example, a component may define a props foo with type number which is required, but if the evaluation throws an error, the component may get undefined as the foo props value.

Possible solution: do not render components in ImplementWrapper when expression evaluation failed.

@tanbowensg
Copy link
Collaborator

I think not-rendering components as a fallback could be too over. How about passing a default value according to the type, like 0 to number, '' to string, false to boolean.

@Yuyz0112
Copy link
Contributor Author

@tanbowensg

For example, we have a component <Bill /> with a props payTheBill. when payTheBill is true, the component will send a request and pay the bill, otherwise, the component will do nothing.

It's quite ok if an application builder writes a wrong expression and accidentally fires the payment request because it's a user error.
But it's not ok if an application builder writes an invalid expression and we fall back to true or false, which accidentally fires the request.

Falling back to any type-right value is dangerous because we do not know the semantics of the props are "pay the bill" or "do not pay the bill".

@tanbowensg tanbowensg added this to the v0.9 milestone Jan 30, 2022
@tanbowensg
Copy link
Collaborator

If we add defaultValue in properties spec, can we solve this problem? We can always fallback to the safe value that is set by the component developer.

@Yuyz0112
Copy link
Contributor Author

@tanbowensg

I think this is an issue of soundness.

What's the expected behavior when a user writes a wrong expression like {{ const a }} and passes the schema to sunmao-ui's runtime?

Responses I've thought about:

  • sunmao-ui's runtime crash and throw the original expression error to the global scope
  • sunmao-ui's runtime catch the error and render a mask UI to display the error
  • sunmao-ui's runtime catch the error in component level and render a mask UI to display the error on top of the corresponding component
  • sunmao-ui's validator catch the error and show a wrapped error message
  • sunmao-ui's runtime catch the error and fallback to defaultValue, then log the error to console

Of course, there may be some different ways to handle this in dev/production environments or in runtime/editor scope, but I think we can start from the specific one.

@tanbowensg tanbowensg modified the milestones: v0.9, v1.0.0 Feb 15, 2022
@tanbowensg tanbowensg modified the milestones: v1.0.0, v0.6 Mar 8, 2022
@tanbowensg
Copy link
Collaborator

Another problem, property value will change after toggle property from normal mode to JS mode.

@MrWindlike
Copy link
Contributor

I think the runtime should catch the error and fall back to the property's defaultValue first. If the property has no defaultValue, then pass the undefined as the component property value.

Maybe it would cause the application to crash, but I think that is fine. Just like running the wrong JavaScript codes in v8 and the application would be crash too.

And in the editor, when the expression fails to eval or the value is wrong, the editor should display the error in the properties form and the component wrapper like the image below.

image

@tanbowensg
Copy link
Collaborator

tanbowensg commented Apr 1, 2022

the editor should display the error in the properties form and the component wrapper like the image below.

Yanzhen worries about it will cause silent bug if we fallback to default value. Maybe displaying error message maybe could avoid this problem.

As to the default value, should we defind default value in spec?

@MrWindlike
Copy link
Contributor

Yanzhen worries about it will cause silent bug if we fallback to default value. Maybe displaying error message maybe could avoid this problem.

I think the component developers should handle the property's value could be undefined condition caused by a wrong expression or empty input and consider whether to provide a default value for the property or throw the errors.

Displaying the error messages could help the Sunmao users fix some expression errors but can't avoid them totally so I think we should handle the wrong expression in some ways.

As to the default value, should we defind default value in spec?

Yes. When switching the JS mode, the expression string is hard to display by the other widgets. Thus, I think maybe set the property to the default value when switching the JS mode.

In addition, I think we should also remain the exampleProperties in the spec because the defaultValue is used to init or reset or be a fallback and the exampleProperties is used to show the base usages of components when inserting the components.

@Yuyz0112
Copy link
Contributor Author

Yuyz0112 commented Apr 6, 2022

I think the component developers should handle the property's value could be undefined condition caused by a wrong expression or empty input and consider whether to provide a default value for the property or throw the errors.

I'm not sure about this. If the component developers declared the props like { value: string } both in typescript and JSON schema, it can be a big flaw that they need to check whether value is undefined and write some fallback code. Especially when they are wrapping existing components to sunmao-ui components.

@Yuyz0112
Copy link
Contributor Author

Yuyz0112 commented Apr 6, 2022

Maybe it would cause the application to crash, but I think that is fine. Just like running the wrong JavaScript codes in v8 and the application would be crash too.

I think this is about when to crash.

For example, when looking at the following code:

function component(props) {
  const UI = doSomething(props)
  return render(UI)
}

function main() {
  const props = evalProps(store)
  return component(props)
}

main()

If there is something happened in the evalProps function, it feels weird the component function can be executed.

@tanbowensg tanbowensg modified the milestones: v0.6, v0.7 May 17, 2022
@tanbowensg tanbowensg removed this from the v0.7 milestone Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor runtime about meta-ui runtime
Projects
None yet
Development

No branches or pull requests

3 participants