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

Issue 26 update to new context api #38

Merged

Conversation

hallenrbx
Copy link
Contributor

@hallenrbx hallenrbx commented Jun 11, 2020

This fix Closes #26

Issue: StoreProvider and Connect used the legacy context API, this change updates it to the new API

Fix: Added a file StoreContext.lua that returns a static Roact Context. StoreProvider now returns a Roact Provider that passes a Rodux Store as its "value". Connect now uses a Roact Consumer to read in the store via props.

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

Had some comments on the approach so far. Let's discuss this later today!

src/StoreProvider.lua Outdated Show resolved Hide resolved
src/StoreProvider.spec.lua Outdated Show resolved Hide resolved
src/connect.lua Outdated Show resolved Hide resolved
src/connect.lua Outdated Show resolved Hide resolved
src/getStore.spec.lua Outdated Show resolved Hide resolved
…rt, removed modification to props, made sure innerProps got passed in as props
…ssue-26-update-to-new-context-api

Merge skeleton and origin
Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

Had some minor followup feedback for some nitpicky details, but the rest of this looks about right.

src/StoreProvider.spec.lua Outdated Show resolved Hide resolved
src/connect.lua Outdated Show resolved Hide resolved
src/connect.lua Outdated Show resolved Hide resolved
src/connect.spec.lua Outdated Show resolved Hide resolved
src/StoreProvider.lua Outdated Show resolved Hide resolved
@LPGhatguy
Copy link
Contributor

One snag that's going to bite us with the new context API is that implementing RoactRodux.UNSTABLE_getStore might not be feasible. I believe we have a few consumers of that API in the app that might require nontrivial refactors to get away from using it.

@github-actions
Copy link

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 30d7893 on hallenrbx:issue-26-update-to-new-context-api into fdaf891 on Roblox:master.

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

Cleaned up the last little details here. I think it's acceptable to remove getStore and cut a semver-incompatible release (0.3.0) to reflect it.

@ZoteTheMighty ZoteTheMighty merged commit 9aa343f into Roblox:master May 21, 2021
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.

Update to new Context API
5 participants