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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Remove safe-member access in expression #1808

Open
bigopon opened this issue Aug 3, 2023 · 12 comments
Open

[RFC] Remove safe-member access in expression #1808

bigopon opened this issue Aug 3, 2023 · 12 comments
Labels
RFC Request For Comment - Potential enhancement that needs discussion and/or requires experimentation

Comments

@bigopon
Copy link
Member

bigopon commented Aug 3, 2023

馃挰 RFC

This RFC aims to align to standard JS how Aurelia template expressions evaluates.

馃敠 Context

At the moment, Aurelia template expressions are evaluated in safe-member access mode. This is the result of Aurelia being developed before the language has a mean to express optional access. For example, the following template will not throw:

<p>${details.name}</p>

even if the component details is null/undefined. When evaluate property name on null/undefined, the result will be undefined. This simplifies mental model dealing with template, as component can just cleanup (setting property values to null/undefined) without having to worry/adding noises to the template (?./?.[]/?.()). Though this is not how normal JS behaves: there should have been a TypeError thrown.

With optional syntax fully supported in the template expression, we can consider ditching this old behavior and align fully with JS. This'll introduce complication for applications migrating from v1. As for applications that rely on this safe-access behavior to avoid breakage, expressions will need to start getting tweaked to use optional syntaxes. This could be a big task for some.

Another way to move forward is to allow both behavior exist at the same time and make the old behavior opt-in via a boolean, this could result in slightly more code but will be a big help for transitioning applications from v1.

@bigopon bigopon added the RFC Request For Comment - Potential enhancement that needs discussion and/or requires experimentation label Aug 3, 2023
@ekzobrain
Copy link
Contributor

Agree that this should be opt-in for some transition period. For example it could be completely removed in 2.1 minor release.

@brandonseydel
Copy link
Member

@bigopon - Can we just grant access to users to do anything on the output of an expression and leave it to just call that func?

bindingOutputExpression: (o:unknown) => o ?? ''

@green3g
Copy link
Contributor

green3g commented Aug 4, 2023

I could see the benefits here. Mainly I think that you would be able to catch errors in templates sooner rather than later by realizing a certain dom element is not displayed properly.

On the other hand, so much data is loaded async and the template doesn't have access to it right away. And many bindings occur on deeply nested properties all of which would require the new.? Syntax to move forward. It would be a little painful to have to type the new syntax in html templates vs just a period.

Personally I think I prefer the v1 ease of use of not having to worry about undefined props to build templates. Many engines follow this format (handlebars, svelte, vuejs) so it could be seen as a con by future adoptors of Aurelia. It would probably be ideal to have an opt in to the old style so moving large applications is not so difficult

@harwee
Copy link

harwee commented Aug 5, 2023

As @roemhildtg mentioned, most template engines out there follow the current syntax, so it would just be a huge entry barrier for new users and will be very difficult to migrate for old users.

As for the potential use case of catching errors in templates that arise due to deeply nested properties, I don't believe it's the responsibility of the template to handle data errors. The template should just render the data it is provided with, if it's valid, else not. The onus of validating the data that is provided to the template should be put on the view model.

Personally, I'd not prefer the new syntax in the template. It just makes the template more complicated and harder to deal with, and I personally cannot see many benefits of migrating away from the current standard in the ecosystem, unless I am missing an obvious advantage of doing so (which I'd love to know)

@ekzobrain
Copy link
Contributor

Strictness is very important thus should be always ON for new apps. It highly improves application behavior expectedness and reduces amount of non-obvious bugs. So it is concidered as a good practice, but of course it may be opt-out.
When outputting a TypeError to the console there may be a note how to switch to non-strict mode if desired.

@bigopon
Copy link
Member Author

bigopon commented Aug 5, 2023

Can we just grant access to users to do anything on the output of an expression and leave it to just call that func?
bindingOutputExpression: (o:unknown) => o ?? ''

Thanks @brandonseydel . We can, but that'll be a separate feature/RFC. Also I think it'll be enabling ad-hoc convention per app, or maybe even per section of an app. Unless you imply something else with the suggestion @brandonseydel.

What we need to have decided is whether the property access a.b.c will throw if a is null, or a.b is null, and whether we have a way to not throw as well. I think so far, we are in favor of having not-throwing as opt-in.

@bigopon
Copy link
Member Author

bigopon commented Aug 5, 2023

Strictness is very important thus should be always ON for new apps. It highly improves application behavior expectedness and reduces amount of non-obvious bugs. So it is concidered as a good practice, but of course it may be opt-out.

Yes, I think I agree with this argument, nicely put 馃憤 .

@green3g
Copy link
Contributor

green3g commented Aug 5, 2023

I thought of follow up questions.

  1. Would the Type Error break the rendering of the application? Let's say a object that you expected to be there gets set to null for some unexpected reason on a production app. Does the app still function?
  2. How detailed could the error messages be? Could it pinpoint to the correct source file line?
  3. Would there be a way to debug / set breakpoint and see what property is causing the error?

One additional thought, if the user does opt in to not using the errors, could the template engine log a warning for undefined properties? I've seen this behavior in CanJS and it is quite useful.

@ekzobrain
Copy link
Contributor

ekzobrain commented Aug 5, 2023

1. Would the Type Error break the rendering of the application? Let's say a object that you expected to be there gets set to null for some unexpected reason on a production app. Does the app still function?

2. How detailed could the error messages be? Could it pinpoint to the correct source file line?

3. Would there be a way to debug / set breakpoint and see what property is causing the error?
  1. It should behave the same as if error was raised by ViewModel
  2. That would be great if template complier is capable to do so
  3. It's unlikely, i think...

One additional thought, if the user does opt in to not using the errors, could the template engine log a warning for undefined properties? I've seen this behavior in CanJS and it is quite useful.

Very good idea! Not just for undefined, but warnings should express the same as TypeError's do, just without raising exception.

@bigopon ?

@harwee
Copy link

harwee commented Aug 7, 2023

Strictness is very important thus should be always ON for new apps. It highly improves application behavior expectedness and reduces amount of non-obvious bugs.

I do agree with this, but also I don't want to break the current method used by most other template engines. Why change something that is not broken?

Would the Type Error break the rendering of the application? Let's say a object that you expected to be there gets set to null for some unexpected reason on a production app. Does the app still function?

This is the only thing I have a problem with new syntax, what would happen after throwing the error? Will the component render or will it just break? I am fine with throwing error in development and break the component but I don't want to break my page in production just because the component couldn't get data in time or got wrong data for whatever reason, a blank component with no content is better than no component and broken layout.

One additional thought, if the user does opt in to not using the errors, could the template engine log a warning for undefined properties? I've seen this behavior in CanJS and it is quite useful.

This is great! I think logging a warning is way better than throwing an error, at least in the initial versions and later convert it to error while adding a flag to revert to warning.

tldr; Throwing error and breaking the component(with new syntax) during development is fine but retaining old behaviour is better for production.

@bigopon
Copy link
Member Author

bigopon commented Aug 8, 2023

One additional thought, if the user does opt in to not using the errors, could the template engine log a warning for undefined properties? I've seen this behavior in CanJS and it is quite useful.

Yeah that'd be simple to add some warnings if applications decided to use the legacy behavior.

How detailed could the error messages be? Could it pinpoint to the correct source file line?

The template compiler is not capable of this in its current form since it'll need to evaluate properties on the view model to do this. We'll leave this job to the extension.

@m-gallesio
Copy link

As @roemhildtg mentioned, most template engines out there follow the current syntax

Though this might be because

This is the result of Aurelia most template engines being developed before the language has a mean to express optional access.

Having relied very often on the viewmodels just ignoring missing properties I think this should definitely be configurable (whether opt-in or opt-out) to ease migrations.
If this is implemented, a setting to determine a template's behaviour on error (throw / raise warning / keep going without messages) seems useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request For Comment - Potential enhancement that needs discussion and/or requires experimentation
Projects
None yet
Development

No branches or pull requests

6 participants