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

Minor 'View Model' conditional issue & data-link with () or without #464

Open
BrentH-Alnico opened this issue Jan 16, 2025 · 8 comments
Open

Comments

@BrentH-Alnico
Copy link

BrentH-Alnico commented Jan 16, 2025

Hi Boris,

I am converting plain objects/arrays project to use View Models.
I had one line that threw me for a loop ;-)

View Model

if tag fails silently to refresh when toggling IsEditing value for instance.

{^{if ~root.IsEditing() && (valueA() == 0 && valueB() == 0 && valueB() == 0)}}

Fix and obvious oversight...remove the wrapping parenthesis (no functional change).

{^{if ~root.IsEditing() && valueA() == 0 && valueB() == 0 && valueB() == 0}}

Plain objects/arrays

No issues here, refresh occurred when toggleing IsEditing with wrapping parenthesis.

{^{if ~root.IsEditing && (valueA == 0 && valueB == 0 && valueB == 0)}}

Here is one of your modified samples...click on Change name to see the issue.
https://jsfiddle.net/alnico/a20ygm81/

To test, click on Change name button.


data-link with () or without

Also, I noticed when making the switch from plain objects/arrays to View Models that properties will work without the () when not defined in the model, is this intended or simply a side effect. If intentional, what is the use case? The data is also included when unmapped. This has me a bit confused ;-)

This works in a View Model: data-link="someCheckbox" no ()

UPDATE: I forgot a person can use View Models with JsRender too...so using it the way I described above with JsViews just does not allow you to use getters and setters?

@BorisMoore
Copy link
Owner

Hi Brent - here is a candidate fix for issues 463 464 465 and 466.
Can you test it in your environment?

jsviews16 Candidate A.js.txt

The wrapping parenthesis should be OK now. (But this was a tricky bug to fix - deep in the parsing code...)

If you are using a ViewModel hierarchy as data, and then you include <input type="checkbox" data-link="someCheckbox" /> or <input data-link="title" /> JsViews will behave the same as if it was a plain object hierarchy, and simply 'patch' the additional someCheckbox or title properties. So you can have your 'opinionated' (strongly typed) VM instances along with additional 'unopinionated' properties, if you so wish :-). Even with JsRender you can add additional 'plain properties' such as title to your VM instances, and then use getters in you JsRender template: {{:title}}. Those patched properties will be conserved also through the unmap() or merge() processes...

@BrentH-Alnico
Copy link
Author

The wrapping parenthesis now work...although of course I never intended, nor will use it that way.
I debated reporting this, but when building, I think it's common to add/remove conditions...and in my case not realizing what I ended up with as my final statement. Something failing silently however is never a good thing ;-)

Thanks for fixing what 'seemed' like a minor issue, much appreciated!


Yes, thanks for confirming what I was thinking regarding VM instances and using properties with or without ().
I will stick with one pattern or the other, not both...
Using VM's...I will remain 'opinionated' ;-)

@BorisMoore
Copy link
Owner

I'm glad you did point out the issue with wrapping parenthesis, since one never knows whether an 'avoidable' issue like that is not hiding an internal coding error which in other scenarios might appear again and not be at all avoidable.

As for opinionated, yes, it make sense to be opinionated when using VMs. But temporary patching in of 'untyped' properties might be useful for some testing scenarios, or whatever... So it's good to know you can do it...

@BrentH-Alnico
Copy link
Author

I found a line of code that broke when using candidate v16...

disabled{:~inArray(~helper.prop(), GUID()) || (~inArray(~helper.prop(), GUID()) && ~helper.prop()^length == 1)}

@BorisMoore
Copy link
Owner

BorisMoore commented Feb 1, 2025

This works for me, with candidate v16A:

<input type="checkbox" data-link="disabled{:~inArray(~helper.prop(), GUID()) || (~inArray(~helper.prop(), GUID()) && ~helper.prop()^length == 1)}"/>

If it fails for you, can you check whether there is some other issue - or provide a small repro?

@BrentH-Alnico
Copy link
Author

BrentH-Alnico commented Feb 1, 2025

Hi Boris,

I sent you test file testCanidate16A_forBoris.html...sorry, no small file.

HTML line in question: 1950 disabled{:~inArray(~People.IsCoHost_list(), GUID()) || (~inArray(~People.IsHost_list(), GUID()) && ~People.IsHost_list()^length == 1)}

Using Candidate v16A

  1. Click radio on Edit / Stats
  2. Click People tab.
  3. Click right-side dot menu for Person B, click checkbox Host, click outside to close menu.
  4. Click right-side dot menu for Person A, checkbox Host should no longer be disable as 1 Host is required...fails silently...
    So, I thought/think this was related to parenthesis parsing issue, I tried disabled{... depends...} and that did not force a refresh.
    However, this did work in v15, see below.

Using v15

Following the same test steps above...in step 4, checkbox Host has now flipped to enabled, click to uncheck it works as expected.
However, you will also get, TypeError: data.MayInviteOthers_list is not a function...issue that you fixed (with candidate v16A).

@BrentH-Alnico
Copy link
Author

BrentH-Alnico commented Feb 6, 2025

Hi Boris,

I am guessing you figured out what the problem was here...it does seem that parsing of parentheses is still an issue...
If I remove the (), the tag refreshes as expected...and both are once again functionally the same as && takes precedence and is processed first...sometimes I forget, rarely think about to much ;-)
However, both implementations should work of course.

Does not work, but should tag does not refresh
disabled{:~inArray(~People.IsCoHost_list(), GUID()) || (~inArray(~People.IsHost_list(), GUID()) && ~People.IsHost_list()^length == 1)}

Works tag refreshes
disabled{:~inArray(~People.IsCoHost_list(), GUID()) || ~inArray(~People.IsHost_list(), GUID()) && ~People.IsHost_list()^length == 1}

UPDATE: disabled{...} does refresh itself, but if it is attached to a checkboxgroup then the change to that array is not triggering all 'other' bindings to the checkboxgroup to refresh...I think ;-)

Best,
Brent

@BorisMoore
Copy link
Owner

I am working on this, or related.

The problem seems to be when there is a final ) at the end of the expression. such as here:

{^{if name() == 'newName' && (valueA() == 'valueA')}}...{{/if}}

Changing valueA() triggers the refresh but changing name() does not.

Adding a depends like this makes the refresh work

{^{if name() == 'newName' && (valueA() == 'valueA') depends="name"}}

...working on it....

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

No branches or pull requests

2 participants