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

feat(store-devtools): change connectOutsideZone to be 'true' by default #4103

Merged

Conversation

va-stefanek
Copy link
Contributor

@va-stefanek va-stefanek commented Oct 29, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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:

Closes #4093

Does this PR introduce a breaking change?

[x] Yes
[] No

@netlify
Copy link

netlify bot commented Oct 29, 2023

Deploy Preview for ngrx-io ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 6de0a2a
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/6544e4c95bcb3b0008722a76
😎 Deploy Preview https://deploy-preview-4103--ngrx-io.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

From an API perspective I don't like having booleans default to true.
Since we're going to provide a migration, could we also rename this property?
Maybe connectInsideZone, with the default value of false.

The migration will add connectInsideZone: true to the config.

@@ -41,6 +48,7 @@ export class StoreDevtools implements Observer<any>, OnDestroy {
public dispatcher: ActionsSubject;
public liftedState: Observable<LiftedState>;
public state: StateObservable;
zone = inject(NgZone);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this added?

@timdeschryver
Copy link
Member

cc @brandonroberts , @markostanimirovic what do you think of a rename?

@brandonroberts
Copy link
Member

cc @brandonroberts , @markostanimirovic what do you think of a rename?

I think your points are reasonable, and the upgrade migration would cover existing projects.

@timdeschryver
Copy link
Member

@va-stefanek sorry, but can you rename the property to connectInsideZone (with the default value of false)?

@markostanimirovic
Copy link
Member

cc @brandonroberts , @markostanimirovic what do you think of a rename?

Agree 👍 IIRC, I initially proposed the connectInZone name for this property.

@timdeschryver
Copy link
Member

connectInZone also works for me 👍

@va-stefanek va-stefanek force-pushed the feature/connectOutsideZone-to-true-4093 branch 2 times, most recently from 557489d to e6eed2a Compare November 2, 2023 19:30
@va-stefanek va-stefanek force-pushed the feature/connectOutsideZone-to-true-4093 branch from e6eed2a to 6de0a2a Compare November 3, 2023 12:17
@brandonroberts brandonroberts changed the title feat(devtools): change connectOutsideZone to be 'true' by default feat(store-devtools): change connectOutsideZone to be 'true' by default Nov 7, 2023
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Thanks @va-stefanek

@timdeschryver timdeschryver merged commit d3b4db0 into ngrx:main Nov 7, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store Devtools: Set default for connectOutsideZone to true
4 participants