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.

Sign up for GitHub

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

Jump to bottom

fix(signals): enable withProps to handle Symbols #4656

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix(signals): enable withProps to handle Symbols #4656

wants to merge 7 commits into from

Conversation

rainerhahnekamp
Copy link
Contributor

Resolve an issue in signalStore that prevented Symbols from being added as property keys when merging
different features.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

The value is just undefined.

Closes #4655

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@rainerhahnekamp
fix(signals): enable withProps to handle Symbols
63c5a4a 
Resolve an issue in `signalStore` that prevented Symbols
from being added as property keys when merging
different features.
@netlify Netlify
Copy link

netlify bot commented Jan 3, 2025 •
edited
Loading

✅ Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 9515620
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/679048554d7a8e0008371ccd

markostanimirovic
markostanimirovic requested changes Jan 14, 2025
View reviewed changes
modules/signals/spec/with-props.spec.ts Outdated Show resolved Hide resolved
modules/signals/src/signal-store.ts Outdated Show resolved Hide resolved
@rainerhahnekamp
fix: remove unnecessary if condition
c747174
@rainerhahnekamp
feat(signals): extend symbol support
655f7c9 
Following functions support symbols:
- `withState`
- `withMethods`
- `withComputed`
- Slices (`DeepSignal`)
- `deepFreeze`
- override protection
@rainerhahnekamp
fix(signals): typing
b85d4cb
@rainerhahnekamp rainerhahnekamp force-pushed the fix/signals/with-props-symbols branch from 05babdd to b85d4cb Compare January 20, 2025 23:32
@markostanimirovic markostanimirovic force-pushed the fix/signals/with-props-symbols branch from 315a336 to 9515620 Compare January 22, 2025 01:22
markostanimirovic
markostanimirovic approved these changes Jan 22, 2025
View reviewed changes
Copy link
Member

@markostanimirovic markostanimirovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Btw, I replaced Object.keys with Reflect.ownKeys in withProps as well and added more tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markostanimirovic markostanimirovic markostanimirovic approved these changes

@timdeschryver timdeschryver Awaiting requested review from timdeschryver

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

withProps doesn't allow Symbols as keys
3 participants
@rainerhahnekamp @markostanimirovic @timdeschryver