Skip to content

Commit

Permalink
fix: ensure that signalify() does not count as reading a dependency, …
Browse files Browse the repository at this point in the history
…to avoid infinite loops. Only a user's explicit read of a property after trying to signalify the property should count as a dependency (namely when signalify() was a no-op because the property was already signalifies before hand elsewhere such as an object from a library)
  • Loading branch information
trusktr committed Dec 16, 2023
1 parent 6435b23 commit e89bbe0
Show file tree
Hide file tree
Showing 7 changed files with 61 additions and 7 deletions.
25 changes: 25 additions & 0 deletions dist/index.test.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/index.test.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/signalify.d.ts.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 5 additions & 2 deletions dist/signalify.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/signalify.js.map

Large diffs are not rendered by default.

24 changes: 24 additions & 0 deletions src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,30 @@ describe('classy-solid', () => {
expect(doer.do()).toBe(123)
})

describe('signalify', () => {
it('is not tracked inside of an effect to prevent loops', () => {
// Library author provides obj
const obj = {n: 123}
signalify(obj, 'n') // library author might signalify obj.n

// User code:
createEffect(() => {
// o.n may or may not already be signalified, user does not know, but they want to be sure they can react to its changes.
signalify(obj, 'n')

obj.n = 123 // does not make an infinite loop

// A deeper effect will be reading the property.
createEffect(() => {
console.log(obj.n)
})
})

// No expectations in this test, the test passes if a maximum
// callstack size error (infinite loop) does not happen.
})
})

it('show that signalify causes constructor to be reactive when used manually instead of decorators', () => {
class Foo {
amount = 3
Expand Down
6 changes: 4 additions & 2 deletions src/signalify.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {getInheritedDescriptor} from 'lowclass'
import {createSignal, $PROXY} from 'solid-js'
import {createSignal, $PROXY, untrack} from 'solid-js'
import type {PropKey, PropSpec} from './decorators/types.js'

const signalifiedProps = new WeakMap<object, Set<string | symbol>>()
Expand Down Expand Up @@ -98,7 +98,9 @@ const isSignalGetter = new WeakSet<Function>()
function createSignalAccessor<T extends object>(
obj: T,
prop: Exclude<keyof T, number>,
initialVal: unknown = obj[prop],
// Untrack here to be extra safe this doesn't count as a dependency and
// cause a reactivity loop.
initialVal: unknown = untrack(() => obj[prop]),
// If an object already has a particular signalified property, override it
// with a new one anyway (useful for maintaining consistency with class
// inheritance where class fields always override fields from base classes
Expand Down

0 comments on commit e89bbe0

Please sign in to comment.