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

[Feature] Add userServerEffect to SDK #664

Closed
wants to merge 14 commits into from

Conversation

bendvc
Copy link
Collaborator

@bendvc bendvc commented Jul 13, 2022

Description

This PR implements the useServerEffect API seen in previous PR's like this one.

The goal of this PR is to create a base API for use to use in building our commerce-sdk-react library where we'll provide react hooks for data fetching of commerce sdk data.

This API can also be used on it's own as an alternative to getProps.

TODO

  • Add feature flag defaulting useServerEffect to false.
  • Remove optional synchronization of async didUpdate functions.
  • Add tests.
  • Make decision on ExpressProvider (e.g. it contains more than express values, maybe a rename is in order).
  • Add shout out to SSE library.
  • Convert code to TypeScript
  • Attempts to remove useUUID

What we won't do?

  • Add the option to chain requests as opposed to running in parallel

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Changes

  • (change1)

How to Test-Drive This PR

  • (step1)

@bendvc bendvc added the wip Work in process label Jul 13, 2022
@bendvc bendvc requested a review from a team as a code owner July 13, 2022 21:43
Copy link
Contributor

@wjhsf wjhsf left a comment

Choose a reason for hiding this comment

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

General notes:

  • object means an empty object with no properties. If you want an object with arbitrary properties, use Record<string, unknown>.
  • {} and Object mean anything except null/undefined. Not just objects. Yeah, confusing!
  • any means "I don't want to do type checking any more", and allows things that should be errors. Prefer unknown to say "I don't know, but still keep me safe!"
  • Record<string, unknown> is equivalent to {[key: string]: unknown}. Just different syntax for different use cases.
  • [] or never[] means "an empty array".
  • Does @types/react define any helper types that we can use, rather than defining our own?

Comment on lines +15 to +16
req: object | undefined // Get type from runtime?
res: object | undefined // Get type from runtime?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
req: object | undefined // Get type from runtime?
res: object | undefined // Get type from runtime?
req?: object // Get type from runtime?
res?: object // Get type from runtime?

req: object | undefined means "req must be defined, but might be explicitly set to undefined (i.e. {req: undefined}).
req?: object means "req may or may actually be there" (i.e. {}).

params: {[key: string]: string}
req: object | undefined // Get type from runtime?
res: object | undefined // Get type from runtime?
[key: string]: object | string | number // This accounts for all the `extraGetPropsArgs` values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[key: string]: object | string | number // This accounts for all the `extraGetPropsArgs` values.
[key: string]: object | string | number | undefined // This accounts for all the `extraGetPropsArgs` values.

Need undefined in the index signature if you have optional properties. 😉

import AppConfig from '../../universal/components/_app-config'

export interface GetPropsArgs {
location: Location
Copy link
Contributor

Choose a reason for hiding this comment

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

[Educational aside] The type Location is inherited from TypeScript's built-in DOM library.

Comment on lines +18 to +22
type ServerEffectState = {
data: object;
loading: boolean;
error: Error;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type ServerEffectState = {
data: object;
loading: boolean;
error: Error;
}
type ServerEffectState<T extends object> = {
data: T;
loading: boolean;
error: Error;
}

Use a generic, otherwise data is just an empty object.

// Type Definitions
type ServerEffectState = {
data: object;
loading: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

SCAPI hooks use isLoading, rather than loading. May want consistency, one way or the other.

* @returns
*/
export const resolveAllEffects = async () => {
const allData = await Promise.all(contexts.map((resolver) => resolver()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const allData = await Promise.all(contexts.map((resolver) => resolver()))
const allData = await Promise.all(contexts.map(async (resolver) => resolver()))

If we somehow end up with a sync resolver that throws an error, using an async method automatically converts that thrown error into a rejected promise. (Not sure if that's something we need to worry about, though.)

}
type DidUpdateFn = (serverEffectArgs: GetPropsArgs) => Promise<any>
type EffectsResolver = () => Promise<{[x: string]: {}}>
type Provider = ({ children, value }: { children: any; value: any; }) => JSX.Element
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you add @types/react as a dep so that JSX gets resolved.


// Globals
export const DEFAULT_CONTEXT_KEY = '__SERVER_EFFECTS__'
const contexts = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const contexts = []
const contexts: YourContextType[] = []

By default, this is assumed to be never[], an array that will never have any elements. Mutations like push won't work.

*/
export const createServerEffectContext = (name: string): ServerEffectContext => {
console.log('--=-=-=---=-=-=-=---==--==-> createServerEffectContext')
const ctxValue = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const ctxValue = {
const ctxValue: WhateverContextValueType = {

Without specifying a type, requests defaults to never[], which is not what we want.

Comment on lines +16 to +18
AppConfig.restore = () => {}

AppConfig.freeze = () => undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is one noop void, but the other undefined?

@bendvc
Copy link
Collaborator Author

bendvc commented Aug 8, 2022

Closing in favour of using react-query

@bendvc bendvc closed this Aug 8, 2022
@wjhsf wjhsf deleted the feature/use-server-effect-utils branch March 17, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work in process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants