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

Refactor async fetch #112

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor async fetch #112

wants to merge 2 commits into from

Conversation

jameseaster
Copy link
Contributor

Resolves #94


What kind of change does this PR introduce?

  • Refactor

What is the current behavior?

  • Render method values are fetched using forEach function

What is the new behavior?

  • Render method values are fetched using .map and Promise.all

Does this PR introduce a breaking change?

  • No

How has this been tested?

  • Locally rendered

Screenshots: n/a

@jameseaster
Copy link
Contributor Author

I'm not sure if this checks all of the boxes from issue #94 , but I wanted to get the solution started.

Interestingly, we actually aren't fetching anything right now. The example credentials all use the template property with large SVGs instead of the url property. Unfortunately, this doesn't change the performance from the UX perspective. The dialog still loads slow when you select a credential, not because it is fetching an image value and taking a long time, but rather from trying to render such large SVG components.

@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.04%. Comparing base (79df04e) to head (daa0e14).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #112   +/-   ##
=======================================
  Coverage   12.04%   12.04%           
=======================================
  Files           6        6           
  Lines          83       83           
=======================================
  Hits           10       10           
  Misses         73       73           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79df04e...daa0e14. Read the comment docs.

Comment on lines 203 to +220
async function getDisplaysFromRenderMethod() {
if(props.credential?.renderMethod?.length) {
props.credential.renderMethod.forEach(async rm => {
if(supportedRenderMethods.includes(rm.type)) {
if(rm.type === 'SvgRenderingTemplate2023') {
useRenderTemplate2023(rm.id);
} else if(rm.type === 'SvgRenderingTemplate2024') {
const {template, url} = rm;
const values = props.credential;
await useRenderTemplate2024(template, url, values);
const imageValues = await Promise.all(
props.credential.renderMethod.map(async rm => {
let imageValue;
if(supportedRenderMethods.includes(rm.type)) {
if(rm.type === 'SvgRenderingTemplate2023') {
imageValue = useRenderTemplate2023(rm.id);
} else if(rm.type === 'SvgRenderingTemplate2024') {
const {template, url} = rm;
const values = props.credential;
imageValue = await useRenderTemplate2024(template, url, values);
}
}
}
});
return imageValue;
})
);
imageValues.filter(Boolean).forEach(v => credentialImages.push(v));
Copy link
Member

Choose a reason for hiding this comment

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

A quick note that none of this is vue-specific code, so we should import a function from a vanilla JS lib and call that (probably from bedrock-web-wallet?). That other library should also be able to, with time use a proxy or use OHTTP to obtain these things -- and be able to interact with a cache to avoid retrieval more than once across components and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlongley
In looking to refactor as described above, I noticed this function calls useRenderTemplate2024 that uses a UI library (Mustache) and a Quasar date library (date) to aid in the UI rendering logic.

Additionally, with renderMethod continuing to change in the future (before its become a stable standard) I thought it'd be best to leave this logic in this vue file for the time being.

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.

Refactor async code used in render methods
3 participants