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

fix(proxy-observation): prevent proxies from being wrapped in proxies again #1716

Merged
merged 3 commits into from
Apr 3, 2023

Conversation

fkleuver
Copy link
Member

Pull Request

📖 Description

If you pass in a property that's part of a computed observation structure into another class, the value is already wrapped in a proxy before that property is observed in the new class. If that new class then also uses computed observation, that property will be wrapped in a proxy again, causing a "proxy within a proxy". This can repeat itself endlessly, causing all sorts of issues.

Simply adding the proxy itself as the key to the proxyMap will solve that problem at least as far as the issue in our app is concerned.

cc @bigopon

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

⏭ Next Steps

@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #1716 (ba72314) into master (f4af700) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1716   +/-   ##
=======================================
  Coverage   88.01%   88.01%           
=======================================
  Files         243      243           
  Lines       22270    22271    +1     
  Branches     5039     5039           
=======================================
+ Hits        19601    19602    +1     
  Misses       2669     2669           
Impacted Files Coverage Δ
...es/runtime-html/src/resources/attribute-pattern.ts 90.72% <ø> (ø)
...kages/runtime-html/src/resources/custom-element.ts 82.70% <ø> (ø)
packages/runtime/src/observation/observable.ts 93.02% <ø> (ø)
packages/kernel/src/di.ts 89.58% <100.00%> (ø)
...kages/runtime/src/observation/proxy-observation.ts 98.66% <100.00%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bigopon bigopon merged commit 7792e9c into master Apr 3, 2023
@bigopon bigopon deleted the proxy-wrapping-issue branch April 3, 2023 09:15
AureliaEffect pushed a commit that referenced this pull request Apr 13, 2023
2.0.0-beta.4 (2023-04-13)

**Features:**

* **debounce-throttle:** flush via signals (#1739) ([af238a9](af238a9))
* **slotted:** add slotted decorator, slotchange bindable for au-slot (#1735) ([8cf87af](8cf87af))
* **router-lite:** extended support for ../ prefix, activeClass router configuration (#1733) ([bd18fde](bd18fde))
* **router-lite:** non-string support for fallback (#1730) ([59da952](59da952))
* **vite-plugin:** add vite plugin (#1726) ([564e533](564e533))
* **router-lite:** ce aliases as configured route (#1723) ([2b7f9fc](2b7f9fc))
* **router-lite:** transitionplan as nav opt ([7905d98](7905d98))

**Bug Fixes:**

* **repeat:** fix mismatchedLengthError on assigning an array with duplicate primitive values (#1737) ([cf60ac8](cf60ac8))
* **vite-plugin:** optionally resolve alias, add preliminary doc (#1731) ([3f37f8d](3f37f8d))
* **select:** insensitive multiple.bind order (#1727) ([c8d912f](c8d912f))
* **ci:** fix vite build in ci, upgrade chromedriver ([564e533](564e533))
* **proxy-observation:** prevent proxies from being wrapped in proxies again (#1716) ([7792e9c](7792e9c))

**Refactorings:**

* **children:** remove children observers from custom element def, make children deco as a hook (#1732) ([5bde983](5bde983))
* **all:** ignore dev message coverage ([5bde983](5bde983))
* **router-lite:** routable fallback ([59da952](59da952))
* **platform:** remove unnecessary properties on PLATFORM (#1722) ([7cd77ad](7cd77ad))
* **router-lite:** route definition configuration ([eba6d61](eba6d61))
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.

None yet

2 participants