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

add sagas for all data layer methods #662

Merged
merged 8 commits into from
Jun 29, 2023
Merged

add sagas for all data layer methods #662

merged 8 commits into from
Jun 29, 2023

Conversation

gonpombo8
Copy link
Contributor

@gonpombo8 gonpombo8 commented Jun 21, 2023

  • Remove all event emitters for fileChange and dirty state (canSave)
  • Add sagas for each rpc method (save, undo, redo, importAsset, etc)
  • Move assets catalog and dirty engine state to redux (app store)
  • Fix types for data-layer rpc host

@gonpombo8 gonpombo8 force-pushed the feat/data-layer-sagas branch from 08c2811 to 72e95f4 Compare June 21, 2023 19:03
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jun 21, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4e1b50a
Status: ✅  Deploy successful!
Preview URL: https://19c2ba55.js-sdk-toolchain.pages.dev
Branch Preview URL: https://feat-data-layer-sagas.js-sdk-toolchain.pages.dev

View logs

SetPreferences = 'set-preferences'
}

let dataLayerInterface: DataLayerRpcClient | undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea of this is to not serialize the dataLayer at all.
All the methods should be called via sagas

state.reconnectAttempts = 0
state.error = undefined
},
error: (state, { payload }: PayloadAction<{ error: ErrorType }>) => {
console.log('[WS] Error', payload.error)
state.error = payload.error
}
},
save: () => {},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will have an action/saga for each method of the dataLayer


async function rpcHandler(serverPort: RpcServerPort<DataLayerContext>) {
// TODO: dataLayer as any
Copy link
Contributor Author

Choose a reason for hiding this comment

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

any type fixed

@github-actions
Copy link
Contributor

github-actions bot commented Jun 21, 2023

Test this pull request

  • The @dcl/sdk package can be tested in scenes by running

    npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/data-layer-sagas/dcl-sdk-7.2.5-5413626063.commit-01cabb6.tgz"
  • To test with npx init

    export SDK_COMMANDS="https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/data-layer-sagas/dcl-sdk-commands-7.2.5-5413626063.commit-01cabb6.tgz"
    npx $SDK_COMMANDS init
  • The @dcl/inspector package can be tested by visiting this url

    • Or by installing it via NPM
    npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/data-layer-sagas/@dcl/inspector/dcl-inspector-7.2.5-5413626063.commit-01cabb6.tgz"
  • The /changerealm command to test test in-world

    /changerealm https://sdk-team-cdn.decentraland.org/ipfs/feat/data-layer-sagas-e2e
    
  • You can preview this build entering:
    https://playground.decentraland.org/?sdk-branch=feat/data-layer-sagas


const FreeCameraInvertRotationIcon = preferences.freeCameraInvertRotation ? BiCheckboxChecked : BiCheckbox
const AutosaveEnabledIcon = preferences.autosaveEnabled ? BiCheckboxChecked : BiCheckbox
console.log('BOEDO: Changed', preferences)
Copy link
Member

Choose a reason for hiding this comment

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

oops

@@ -23,9 +25,9 @@ export const useSdkContext = () => {
}, [dispatch])

useEffect(() => {
if (!catalog || !canvas || sdk || isLoading || !dataLayer) return
if (!catalog || !canvas || sdk || isLoading || !dataLayer || !preferences) return
Copy link
Member

Choose a reason for hiding this comment

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

do we need the !dataLayer if it's not used anymore by createSdkContext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, i think that we are covered with the prefences. good catch

@@ -35,7 +37,7 @@ export const useSdkContext = () => {
setError(e)
})
.finally(() => setIsLoading(false))
}, [catalog, canvas, sdk, isLoading, dispatch, dataLayer])
}, [catalog, canvas, sdk, isLoading, dispatch, dataLayer, preferences])
Copy link
Member

Choose a reason for hiding this comment

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

Same as above with the dataLayer here

state.canSave = isDirty
},
updatePreferences: (state, { payload }: PayloadAction<{ preferences: InspectorPreferences }>) => {
console.log(' UPDATED ', payload.preferences)
Copy link
Member

Choose a reason for hiding this comment

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

log

export const getInspectorPreferences = (state: RootState): InspectorPreferences => {
if (!state.app.preferences) {
// TODO: send to sentry
console.log('invalid inspector preferences')
Copy link
Member

Choose a reason for hiding this comment

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

log

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 57.27% and project coverage change: -0.09 ⚠️

Comparison is base (f97bfb9) 73.46% compared to head (4e1b50a) 73.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #662      +/-   ##
==========================================
- Coverage   73.46%   73.37%   -0.09%     
==========================================
  Files         289      296       +7     
  Lines        9661     9815     +154     
  Branches     1274     1277       +3     
==========================================
+ Hits         7097     7202     +105     
- Misses       2443     2491      +48     
- Partials      121      122       +1     
Impacted Files Coverage Δ
...s/@dcl/inspector/src/hooks/catalog/useAssetTree.ts 0.00% <0.00%> (ø)
...spector/src/redux/data-layer/sagas/remove-asset.ts 0.00% <0.00%> (ø)
.../inspector/src/redux/data-layer/sagas/undo-redo.ts 17.07% <17.07%> (ø)
packages/@dcl/inspector/src/lib/sdk/context.ts 47.82% <20.00%> (+1.15%) ⬆️
...ages/@dcl/inspector/src/hooks/sdk/useSdkContext.ts 25.49% <22.22%> (+1.96%) ⬆️
...edux/data-layer/sagas/set-inspector-preferences.ts 24.00% <24.00%> (ø)
.../sdk-commands/src/commands/start/data-layer/rpc.ts 41.66% <28.57%> (ø)
...spector/src/redux/data-layer/sagas/import-asset.ts 29.41% <29.41%> (ø)
.../@dcl/inspector/src/redux/data-layer/sagas/save.ts 33.33% <33.33%> (ø)
...or/src/redux/data-layer/sagas/get-asset-catalog.ts 37.50% <37.50%> (ø)
... and 17 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gonpombo8 gonpombo8 force-pushed the feat/data-layer-sagas branch 2 times, most recently from 0d9c17f to dcba58f Compare June 27, 2023 16:27
@gonpombo8 gonpombo8 changed the title add sagas for InspectorPreferences and Save methods add sagas for all data layer methods Jun 27, 2023
@gonpombo8 gonpombo8 marked this pull request as ready for review June 27, 2023 18:42

const fixedNumber = (val: number) => Math.round(val * 1e2) / 1e2

const Renderer: React.FC = () => {
const canvasRef = React.useRef<HTMLCanvasElement>(null)
useRenderer(() => canvasRef)
const sdk = useSdk()
const dataLayer = useAppSelector(getDataLayer)

const dispatch = useAppDispatch()
const [isLoading, setIsLoading] = useState(false)
Copy link
Member

Choose a reason for hiding this comment

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

This loading state might need to be controlled from the saga, otherwise right now it set to true and then to false right after the action is dispatched (because the dispatch does not await)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do it in another pr :)

undo: () => {},
redo: () => {},
importAsset: (_state, _payload: PayloadAction<ImportAssetRequest>) => {},
removeAsset: (_state, _payload: PayloadAction<Asset>) => {}
Copy link
Member

Choose a reason for hiding this comment

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

are these above unsued?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, this generates the action with that payload type.
So then in the saga you can listen to this importAsset action

@gonpombo8 gonpombo8 force-pushed the feat/data-layer-sagas branch from e40f5b5 to 4e1b50a Compare June 29, 2023 14:50
@gonpombo8 gonpombo8 merged commit d72f900 into main Jun 29, 2023
@gonpombo8 gonpombo8 deleted the feat/data-layer-sagas branch June 29, 2023 15:04
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.

2 participants