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(template): auto infer binding expression when empty #1963

Merged
merged 3 commits into from
May 8, 2024

Conversation

bigopon
Copy link
Member

@bigopon bigopon commented May 7, 2024

📖 Description

Our binding syntax comprises of 3 parts: target, command and expression as per the following formular:

target.command="expression"

Currently, when expression is absent, or empty, like the following examples:

<input value.bind="">
or
<input value.bind>

it's treated as an empty string, which means the template above works like this:

<input value.bind="``">

This is a missed-opportunity in v1, where we could have better dev experience, like suggested in #1259 . When the expression is either empty or absent, we can infer the expression based on the target. so for the above example, it should be understood as:

<input value.bind="value">

All binding commands that will share this behavior are:

  • .bind
  • .one-time
  • .to-view
  • .from-view
  • .two-way
  • .attr

This is technically a breaking change, but no-one would write anything meaningful with empty expression so the effect of this should be small, or none. Though it's still safer to have this done in beta so we don't cause unnecessary discomfort.

🎫 Issues

Close #1259

cc @fkleuver @Sayan751 @brandonseydel

Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (eaf2cd7) to head (98b3d43).
Report is 2 commits behind head on master.

❗ Current head 98b3d43 differs from pull request most recent head ebc8f0c. Consider uploading reports for the commit ebc8f0c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1963      +/-   ##
==========================================
- Coverage   88.62%   88.54%   -0.08%     
==========================================
  Files         272      272              
  Lines       22823    22812      -11     
  Branches     5292     5281      -11     
==========================================
- Hits        20226    20200      -26     
- Misses       2597     2612      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bigopon bigopon merged commit 3359939 into master May 8, 2024
4 of 6 checks passed
@bigopon bigopon deleted the feat/default-binding-expression branch May 8, 2024 00:39
AureliaEffect pushed a commit that referenced this pull request May 11, 2024
2.0.0-beta.17 (2024-05-11)

**BREAKING CHANGES:**

* **template:** auto infer binding expression when empty (#1963) ([3359939](3359939))

    Previously only the expression of binding to element bindables get auto inferred, now it's expanded to all bindings
    with `.bind`/`.to-view`/`.from-view`/`.two-way`/`.one-time`
    Examples:
    ```html
    <div some-prop.bind=""> means <div some-prop.bind="someProp">
    <div some-prop.bind> means <div some-prop.bind="someProp">
    <div some-prop.one-time> means <div some-prop.one-time="someProp">
    ...
    ```
* **convention:** rewrite runtime-html decorators (#1960) ([eaf2cd7](eaf2cd7))

    With tooling in the instable state for the tc39 decorator support, we will generate standard fn call code instead of decorator.
    This will likely be changed when browsers start officially supporting it, or at least when the tooling (both spec & tooling stability + compat) gets better

**Features:**

* **template:** support spread syntax with `spread` command and ... (#1965) ([ccae63b](ccae63b))
* **repeat:** allow custom repeatable value (#1962) ([c47df91](c47df91))

**Bug Fixes:**

* **compiler:** fix order when spreading custom attribute into element bindable ([ccae63b](ccae63b))
* **au-slot:** separate parent scope selection from host scope selection (#1961) ([ff605fb](ff605fb))

**Refactorings:**

* **kernel:** mark side effect free (#1964) ([22c8f71](22c8f71))
@ghiscoding
Copy link
Contributor

ghiscoding commented May 12, 2024

@bigopon I'm trying to upgrade to Beta 17 and I'm getting a few errors as shown below in my Aurelia-Slickgrid demos, all of the code that fails have the @bindable decorator in this CI Failing Workflow in this PR

TS2488: Type 'Record<string, PartialBindableDefinition>' must have a 'Symbol.iterator' method that returns an iterator.

I don't exactly understand what I'm supposed to do in order to migrate? If I remove all the @bindable then it compiles without error but all my E2E tests then fail.

Here's code for 1 example that I have (code ref link)

export class Example21 {
  @bindable() selectedColumn!: Column;
  @bindable() selectedOperator!: string;
  @bindable() searchValue = '';

Do you have a clue what the errors are all about? Taking a look at the Bindable docs, it seems all the same.

@bigopon
Copy link
Member Author

bigopon commented May 12, 2024

@ghiscoding this likely is a typing issur. Can you try bindable() as any to see if theres any runtime error?

@bigopon
Copy link
Member Author

bigopon commented May 12, 2024

Nvm, sorry I forgot we rewrite decorator code with convention plugin. This likely is a types issues from rewritten code.

@bigopon
Copy link
Member Author

bigopon commented May 12, 2024

@ghiscoding we have a test for this scenario too in our e2e, though a difference is in our setup we use transpileOnly here https://github.com/aurelia/aurelia/blob/master/packages/__e2e__/3-hmr-webpack/webpack.config.js#L36-L42
This is the reason why we didn't catch the issue. For now, you can apply the same change to temporarily work around it, will fix soon. Thanks!

@obedm503 you don't like this change?

@obedm503
Copy link

I don't like implicit behavior. An empty binding should result in an error. Even something.bind="" should be an error instead of an empty string.

@ghiscoding
Copy link
Contributor

@ghiscoding we have a test for this scenario too in our e2e, though a difference is in our setup we use transpileOnly here https://github.com/aurelia/aurelia/blob/master/packages/__e2e__/3-hmr-webpack/webpack.config.js#L36-L42 This is the reason why we didn't catch the issue. For now, you can apply the same change to temporarily work around it, will fix soon. Thanks!

@obedm503 you don't like this change?

Thanks that worked, even if it wasn't an obvious fix. 👍🏻

@bigopon
Copy link
Member Author

bigopon commented May 12, 2024

... implicit behavior. An empty binding should result in an error ...

I think when faced with an empty string, we have 2 choices: strict (throw) and helpful (infer).

We can choose to be strict, but I think being helpful would be nicer, especially if it's not obscure, which is the case for our change here. Whatever the expression is can be seen from the attribute, there's nothing hidden at all.

@ghiscoding
Copy link
Contributor

We can choose to be strict, but I think being helpful would be nicer, especially if it's not obscure, which is the case for our change here

I agree which is why I wrote here because I wasn't sure what to do since the message was confusing

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.

[RFC] Ability to not double specify on bindable match on name
3 participants