Add fix for issue 2996 #3046

puckowski · 2022-07-16T22:43:41Z

Add fix for issue 2996. Watch magic had an issue with value reference.

Should address: #2996

Cypress tests replicated here and all passing using the patch:
https://codepen.io/puckowski/pen/poLRwEr?editors=1111
39 assertions passing.

User reported in #2996 that the fix solved their issue.

* Add fix for issue 2996. Watch magic had an issue with value reference.

puckowski · 2022-07-16T22:50:18Z

Fix works for user reported Code Pen at: https://codepen.io/dinyo/pen/eYVaKgj?editors=1111
User Code Pen from: #2996

stepanjakl · 2022-07-26T15:12:24Z

I am facing the same issue this.$watch('filter.active', (value, oldValue) => { this.onFilterChange(value, oldValue) }), where deeply nested values don't retain the oldValue. This PR should help.

ovenum · 2022-08-05T08:51:46Z

Thanks for creating this pull request @puckowski.
Currently also entering a hole-of-dev-doom when using $watch.… : )

I’ve looked at the PR and when calling JSON.parse(JSON.stringify(value)) on an array of File objects it will return an array of empty JS objects. This might not be what people using it are expecting.

If this PR gets accepted, the documenation should explain the capabilites/limitation for $watch a bit better.

SimoTod · 2022-08-05T10:58:07Z

Most recent browsers support structuredClone but it's quite new and you would need a polyfill for the rest (https://github.com/zloirock/core-js#structuredclone). I suppose $watch could warn the user that they should include a polyfill when watching an object then use structuredClone if it exists or your solution otherwise.

SimoTod · 2022-08-05T11:01:52Z

packages/alpinejs/src/magics/$watch.js

@@ -17,10 +17,10 @@ magic('watch', (el, { evaluateLater, effect }) => (key, callback) => {
            queueMicrotask(() => {
                callback(value, oldValue)

-                oldValue = value
+                oldValue = JSON.parse(JSON.stringify(value))


JSON.stringify is expensive, the code already does it at line 12 so you can just assign it to a temp variable and reuse that in here?

SimoTod · 2022-08-05T11:02:25Z

packages/alpinejs/src/magics/$watch.js

            })
        } else {
-            oldValue = value
+            oldValue = JSON.parse(JSON.stringify(value))


same as above

* Use strucutredClone for improved watch to avoid object by reference issues.

puckowski · 2022-08-06T14:37:15Z

I reworked my fix to use structuredClone, but it seems Cypress doesn't have access to structuredClone some some checks have failed with the latest push.

Users would need to polyfill structuredClone for older browser support from core-js (https://www.npmjs.com/package/core-js) in order for this solution to work.

…2996

* Add warning and try to update Cypress.

* Update focus-trap to version 6.9.4.

* Try to fix focus trap when no autofocus defined.

* Working on focus trap.

* Fixes for focus-trap.

puckowski · 2022-08-06T16:38:21Z

Will scrutinize the changes I have made later today. Had to update Cypress to get access to structuredClone, and also updated focus-trap with a minor code change in the focus index.js.

* Add Cypress test for watch nested arrays. * Add test for structuredClone support and warn. * Add comments for focus handling of initial focus.

* Refactor to use method to reduce duplicate lines of code.

puckowski · 2022-08-07T16:17:17Z

To summarize, I used structuredClone instead of JSON.parse(JSON.stringify()) which means that users of watch will need to polyfill structuredClone for older browser support. I log a warning if structuredClone is not defined on the window object.

I had to update Cypress from 5.5.0 to 9.7.0 in order for the tests to pass (newer version of browser with structuredClone support).

Updating Cypress seems to have broken some tests for focus, so I updated focus-trap to 6.9.4 from 6.6.1 and made a small code change with comments to focus index.js.

@SimoTod

Cypress tests passing and passing in this CodePen.
https://codepen.io/puckowski/pen/poLRwEr?editors=1111

Accidentially closed this pull request for a moment. Still would like to see it merged.

FIx also still works for https://codepen.io/dinyo/pen/eYVaKgj?editors=1111

* Improve watch edge cases when trying strucutredClone on some proxied objects. * Add watch test for array with objects.

puckowski · 2022-08-09T18:57:07Z

Simple watch case working in https://codepen.io/puckowski/pen/VwXBKWZ?editors=1111
All watch tests passing, plus some logic for watching File objects working in https://codepen.io/puckowski/pen/VwXBKWZ?editors=1111 (CodePen does not fully log File objects, add the code to a local IDE to test more thoroughly).
Added test for watch with arrays containing objects.
Removed unnecessary JSON.stringify call for performance.
Added improved handling for cloning some proxied objects. Prefer structuredClone first. If that fails, shallow clone a new object and then perform a structuredClone on the shallow copy. Logic is working.

njpanderson · 2022-08-21T18:56:12Z

packages/alpinejs/src/magics/$watch.js

-        // JSON.stringify touches every single property at any level enabling deep watching
-        JSON.stringify(value)
+    const shallowClone = (obj) => {
+        if (typeof obj !== 'object') {


Hi all — just implemented this PR in my own project and got a JS error for my troubles. It was due to this line assuming that if obj is an object, then it can be parsed by Object.keys — This is incorrect, as null is also an object, but cannot be parsed:

Object.keys(null) > Uncaught TypeError: can't convert null to object

I would recommend you add another check for null, e.g:

if ( typeof obj !== 'object' || obj === null ) {

(There may be other clauses needed here but this one got it working for me).

Also: Is it odd that it tested? Apologies, but I didn't have time to look at your test suites.

(Not doing this as an official review as I don't know your contrib process. Sorry if I'm stepping on toes here!)

Apologies guys but I've also noticed that the watcher only seems to detect the change in value once. No subsequent changes are detected. I've not got a lot of time to look into this properly but thought you should know regardless. Thanks!

* Add support for edge case with null object. * Add two tests for watch for Cypress that deal with null object.

* Update Cypress magic tests to ensure watched value is updated.

* Update Cypress magic tests. Verify old value is being updated correctly.

* Update Cypress magic tests.

* Fix callback not being triggered.

…main_2996

* Update focus for tests.

* Revert focus on element change.

puckowski · 2022-08-21T23:44:18Z

Synced fork with latest main (08/21/22).
Added support for null case and added two Cypress tests dealing with null cases, which are passing.
https://codepen.io/puckowski/pen/PoRLZNO?editors=1111 demonstrates latest CDN build updates new and old values correctly

@njpanderson Let me know if this resolves your issue.

Old:

Simple watch case working in https://codepen.io/puckowski/pen/VwXBKWZ?editors=1111
All watch tests passing, plus some logic for watching File objects working in https://codepen.io/puckowski/pen/VwXBKWZ?editors=1111 (CodePen does not fully log File objects, add the code to a local IDE to test more thoroughly).
Added test for watch with arrays containing objects.
Removed unnecessary JSON.stringify call for performance.
Added improved handling for cloning some proxied objects. Prefer structuredClone first. If that fails, shallow clone a new object and then perform a structuredClone on the shallow copy. Logic is working.

njpanderson · 2022-08-22T07:06:56Z

I've given the latest push a few run throughs @puckowski and not seeing any further issues. Nice work!

kenset · 2022-10-08T13:25:13Z

Apologies if this is not the right place to ask, but is there a timeline for this being merged and released?

ekwoka · 2022-10-08T13:57:44Z

@kenset

No. Caleb has been very busy with the Livewire rewrite and has a been a bit of an absent father to little Alpine. It may not be until after the new Livewire is released.

joshhanley · 2022-11-23T22:27:38Z

Thanks for the PR and all the support!

It's well thought out and makes sense. There are a couple of concerns with this PR. One is the size of the implementation, it adds quite a few lines of code, which increases the package size. And the other is using an API that is less than a year old, which may cause issues for end users.

We're going to leave it open for the moment, so it can be reviewed in detail.

joshhanley · 2023-01-28T00:29:56Z

As a further follow up, we're going to leave this PR open for now, as Caleb wants to review it in detail, but currently doesn't have time whilst working on Livewire V3.

Add fix for issue 2996

f20d6c1

* Add fix for issue 2996. Watch magic had an issue with value reference.

SimoTod reviewed Aug 5, 2022

View reviewed changes

Use structuredClone for improved watch

c145459

* Use strucutredClone for improved watch to avoid object by reference issues.

puckowski added 6 commits August 6, 2022 11:21

Merge branch 'main' of https://github.com/puckowski/alpine into main_…

fac4d64

…2996

Add warning and try to update Cypress

caeeeed

* Add warning and try to update Cypress.

Update focus-trap package

8bc5079

* Update focus-trap to version 6.9.4.

Try to fix focus trap when no autofocus defined

3860640

* Try to fix focus trap when no autofocus defined.

Working on focus trap

0180509

* Working on focus trap.

Fixes for focus-trap

23449a5

* Fixes for focus-trap.

puckowski added 2 commits August 6, 2022 20:01

Fixes and comments added

a5cc90b

* Add Cypress test for watch nested arrays. * Add test for structuredClone support and warn. * Add comments for focus handling of initial focus.

Refactor to use method

e9c4c24

* Refactor to use method to reduce duplicate lines of code.

puckowski closed this Aug 7, 2022

puckowski reopened this Aug 7, 2022

Improve watch edge cases and add test

1a1539f

* Improve watch edge cases when trying strucutredClone on some proxied objects. * Add watch test for array with objects.

njpanderson reviewed Aug 21, 2022

View reviewed changes

puckowski added 7 commits August 21, 2022 17:25

Add support for edge case with null object

a0a2867

* Add support for edge case with null object. * Add two tests for watch for Cypress that deal with null object.

Update Cypress magic tests

fe7ef76

* Update Cypress magic tests to ensure watched value is updated.

update Cypress magic tests

add8f92

* Update Cypress magic tests. Verify old value is being updated correctly.

Update Cypress magic tests

16f8bbd

* Update Cypress magic tests.

Merge branch 'alpinejs:main' into main_2996

fbd6549

Fix callback not being triggered

19fc12f

* Fix callback not being triggered.

Merge branch 'main_2996' of https://github.com/puckowski/alpine into …

444043f

…main_2996

puckowski added 2 commits August 21, 2022 19:24

Update focus for tests

4749528

* Update focus for tests.

Revert focus on element change

4bbd703

* Revert focus on element change.

puckowski mentioned this pull request Aug 23, 2022

$watch array output the same value between old and new. #2996

Closed

joshhanley added the on hold label Jan 28, 2023

puckowski mentioned this pull request Jun 10, 2023

Add enhancement for discusssion #3293 #3617

Closed

calebporzio closed this Jan 20, 2024

Suall1969 mentioned this pull request Oct 19, 2024

Fix getting the old value in deep watching #4406

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fix for issue 2996 #3046

Add fix for issue 2996 #3046

puckowski commented Jul 16, 2022

puckowski commented Jul 16, 2022

stepanjakl commented Jul 26, 2022

ovenum commented Aug 5, 2022 •

edited

Loading

SimoTod commented Aug 5, 2022 •

edited

Loading

SimoTod Aug 5, 2022

SimoTod Aug 5, 2022

puckowski commented Aug 6, 2022 •

edited

Loading

puckowski commented Aug 6, 2022

puckowski commented Aug 7, 2022 •

edited

Loading

puckowski commented Aug 9, 2022

njpanderson Aug 21, 2022

njpanderson Aug 21, 2022

njpanderson Aug 21, 2022 •

edited

Loading

puckowski commented Aug 21, 2022

njpanderson commented Aug 22, 2022

kenset commented Oct 8, 2022

ekwoka commented Oct 8, 2022

joshhanley commented Nov 23, 2022

joshhanley commented Jan 28, 2023

Add fix for issue 2996 #3046

Add fix for issue 2996 #3046

Conversation

puckowski commented Jul 16, 2022

puckowski commented Jul 16, 2022

stepanjakl commented Jul 26, 2022

ovenum commented Aug 5, 2022 • edited Loading

SimoTod commented Aug 5, 2022 • edited Loading

SimoTod Aug 5, 2022

Choose a reason for hiding this comment

SimoTod Aug 5, 2022

Choose a reason for hiding this comment

puckowski commented Aug 6, 2022 • edited Loading

puckowski commented Aug 6, 2022

puckowski commented Aug 7, 2022 • edited Loading

puckowski commented Aug 9, 2022

njpanderson Aug 21, 2022

Choose a reason for hiding this comment

njpanderson Aug 21, 2022

Choose a reason for hiding this comment

njpanderson Aug 21, 2022 • edited Loading

Choose a reason for hiding this comment

puckowski commented Aug 21, 2022

njpanderson commented Aug 22, 2022

kenset commented Oct 8, 2022

ekwoka commented Oct 8, 2022

joshhanley commented Nov 23, 2022

joshhanley commented Jan 28, 2023

ovenum commented Aug 5, 2022 •

edited

Loading

SimoTod commented Aug 5, 2022 •

edited

Loading

puckowski commented Aug 6, 2022 •

edited

Loading

puckowski commented Aug 7, 2022 •

edited

Loading

njpanderson Aug 21, 2022 •

edited

Loading