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

chore(web): cleanup promise handling #7382

Merged
merged 31 commits into from
Feb 27, 2024
Merged

chore(web): cleanup promise handling #7382

merged 31 commits into from
Feb 27, 2024

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Feb 23, 2024 •
edited
Loading

These are a subset of the errors pointed out by eslint when setting recommended-type-checked.

@benmccann benmccann changed the title chore: cleanup promise handling chore(web): cleanup promise handling Feb 23, 2024
@cloudflare-pages Cloudflare Pages
Copy link

cloudflare-pages bot commented Feb 24, 2024 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 21fa9d3
Status:⚡️  Build in progress...

View logs

mertalev
mertalev reviewed Feb 24, 2024
View reviewed changes
Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

Looks really good! I'm surprised there are so many places where we don't await

web/src/lib/utils.ts Show resolved Hide resolved
web/src/routes/admin/jobs-status/+page.svelte Outdated Show resolved Hide resolved
michelheusschen
michelheusschen reviewed Feb 26, 2024
View reviewed changes
Copy link
Contributor

@michelheusschen michelheusschen left a comment

Choose a reason for hiding this comment

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

Will it still be possible to call an async function in onMount and (without awaiting) return a cleanup function? For example to abort a request.

web/src/routes/admin/jobs-status/+page.svelte Outdated Show resolved Hide resolved
web/src/routes/(user)/albums/[albumId]/+page.svelte Outdated Show resolved Hide resolved
web/src/lib/utils.ts Outdated Show resolved Hide resolved
@benmccann benmccann mentioned this pull request Feb 26, 2024
Merged
jrasm91
jrasm91 reviewed Feb 26, 2024
View reviewed changes
web/src/lib/components/asset-viewer/photo-viewer.svelte Show resolved Hide resolved
web/src/routes/(user)/trash/+page.svelte Outdated Show resolved Hide resolved
web/src/routes/auth/change-password/+page.svelte Outdated Show resolved Hide resolved
web/src/routes/auth/login/+page.svelte Show resolved Hide resolved
web/svelte.config.js Show resolved Hide resolved
jrasm91
jrasm91 approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Code looks good, but it would be good to do some basic testing as well.

@benmccann
Copy link
Contributor Author

I manually tested the app by navigating around and trying out various pieces of functionality. The only thing I found that looked like a bug also happens on main. When you click the heart to favorite or un-favorite a photo, that photo flashes and disappears for a second, which seemed unexpected to me.

@alextran1502 alextran1502 merged commit 907a95a into main Feb 27, 2024
24 of 25 checks passed
@alextran1502 alextran1502 deleted the eslint branch February 27, 2024 16:37
@benmccann benmccann mentioned this pull request Feb 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michelheusschen michelheusschen michelheusschen left review comments

@jrasm91 jrasm91 jrasm91 approved these changes

@mertalev mertalev mertalev approved these changes

Assignees
No one assigned
Labels
🖥️web
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@benmccann @jrasm91 @michelheusschen @mertalev @alextran1502