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

#182 SSR data is shared #375

Merged
merged 14 commits into from
May 5, 2021
Merged

#182 SSR data is shared #375

merged 14 commits into from
May 5, 2021

Conversation

Munawwar
Copy link
Contributor

@Munawwar Munawwar commented May 1, 2021

No description provided.

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 1, 2021 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3d882e3:

Sandbox Source
React Configuration
React Typescript Configuration

@Munawwar Munawwar changed the title Issue 182 ssr #182 SSR is shared May 1, 2021
@Munawwar
Copy link
Contributor Author

Munawwar commented May 1, 2021 •
edited
Loading

Pending work & questions:

  • there is a FIXME on context.ts, on useStore's type assertion. Seems like an issue with existing type for useStore?
  • Update README on how to use the new context APIs. I'd like someone to help me out here.
  • Should we add a next.js example folder in the repo? or should we just add a link to the next.js canary repo?

I will try to add example to next.js canary repo after we release this.

@Munawwar Munawwar changed the title #182 SSR is shared WIP: #182 SSR is shared May 1, 2021
@Munawwar Munawwar marked this pull request as draft May 1, 2021 08:17
@Munawwar Munawwar mentioned this pull request May 1, 2021
Closed
dai-shi
dai-shi reviewed May 1, 2021
View reviewed changes
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Some early feedbacks.

src/context.ts Outdated Show resolved Hide resolved
src/context.ts Outdated Show resolved Hide resolved
src/context.ts Outdated Show resolved Hide resolved
src/context.ts Outdated Show resolved Hide resolved
@dai-shi
Copy link
Member

dai-shi commented May 1, 2021

Should we add a next.js example folder in the repo? or should we just add a link to the next.js canary repo?

I think just adding a link is fine, assuming we update the example in the nextjs repo.

@Munawwar
Copy link
Contributor Author

Munawwar commented May 1, 2021 •
edited
Loading

@dai-shi Ah, sorry.. I didn't push my changes properly last time (some last minute force push removed my new changes). Can you check again?

@Munawwar Munawwar changed the title WIP: #182 SSR is shared WIP: #182 SSR data is shared May 1, 2021
dai-shi
dai-shi reviewed May 1, 2021
View reviewed changes
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Overall it looks good! Added some comments.

src/context.ts Outdated Show resolved Hide resolved
src/context.ts Outdated Show resolved Hide resolved
tests/context.test.tsx Outdated Show resolved Hide resolved
tests/context.test.tsx Show resolved Hide resolved
dai-shi
dai-shi reviewed May 1, 2021
View reviewed changes
src/context.ts Outdated Show resolved Hide resolved
@Munawwar
change 'createState' prop to 'initialStore'
db2be8b 
that accepts useStore func/object from create().

This is needed as store access is needed for merging/memoizing, in next.js integration
@Munawwar
updated tests
8566396
@Munawwar
code review changes
1bd9110
@Munawwar Munawwar changed the title WIP: #182 SSR data is shared #182 SSR data is shared May 2, 2021
@Munawwar Munawwar marked this pull request as ready for review May 2, 2021 18:39
dai-shi
dai-shi approved these changes May 2, 2021
View reviewed changes
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice work.

@dai-shi dai-shi linked an issue May 2, 2021 that may be closed by this pull request
SSR Example (SSR data is shared) #182
Closed
@dai-shi dai-shi merged commit 611551c into pmndrs:master May 5, 2021
@dai-shi dai-shi mentioned this pull request May 5, 2021
Closed
@dai-shi dai-shi mentioned this pull request Jun 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dai-shi dai-shi dai-shi approved these changes

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

Successfully merging this pull request may close these issues.

SSR Example (SSR data is shared)
2 participants
@Munawwar @dai-shi