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

Make SaveContext provides access to side effects #5604

Merged
merged 5 commits into from Dec 4, 2020
Merged

Make SaveContext provides access to side effects #5604

merged 5 commits into from Dec 4, 2020

Conversation

djhi
Copy link
Contributor

@djhi djhi commented Nov 30, 2020

No description provided.

@djhi djhi added the RFR Ready For Review label Nov 30, 2020
@djhi djhi added this to the 3.10.3 milestone Nov 30, 2020
@djhi djhi added WIP Work In Progress and removed RFR Ready For Review labels Nov 30, 2020
@fzaninotto fzaninotto removed this from the 3.10.3 milestone Nov 30, 2020
@djhi djhi added RFR Ready For Review and removed WIP Work In Progress labels Dec 1, 2020
@djhi djhi added this to the 3.10.4 milestone Dec 1, 2020
@djhi
Copy link
Contributor Author

djhi commented Dec 1, 2020

This PR should probably target next, shouldn't it?

fzaninotto
fzaninotto requested changes Dec 2, 2020
View reviewed changes
Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Yes, this should target next. Besides, the linter has some remarks about missing dependencies in your useEffect calls in tests.

packages/ra-core/src/controller/details/SaveContext.tsx
@@ -24,7 +27,7 @@ interface SaveContextValue extends SideEffectContextValue {
saving?: boolean;
}

export const SaveContext = createContext<SaveContextValue>({});
export const SaveContext = createContext<SaveContextValue>(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised TypeScript doesn't complain about the default context value. The type should be SaveContextValue? I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was surprised as well but yet it doesn't complain about it

@djhi djhi force-pushed the save-context-value-side-effects-access branch from 0b0ac01 to e465de7 Compare December 3, 2020 07:41
@djhi djhi changed the base branch from master to next December 3, 2020 07:41
@fzaninotto fzaninotto modified the milestones: 3.10.4, 3.11 Dec 4, 2020
@fzaninotto fzaninotto merged commit 02bbccf into next Dec 4, 2020
@fzaninotto fzaninotto deleted the save-context-value-side-effects-access branch December 4, 2020 10:41
@fzaninotto fzaninotto mentioned this pull request Dec 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fzaninotto fzaninotto fzaninotto approved these changes

Assignees
No one assigned
Labels
RFR Ready For Review
Projects
None yet
Milestone
3.11
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@djhi @fzaninotto