-
Notifications
You must be signed in to change notification settings - Fork 39
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 csv export for bookmarks #118
Changes from 2 commits
80e9e68
46b910c
28e0918
0784c9d
e0fe24b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,7 @@ | ||
import express from 'express'; | ||
/* eslint-disable import/no-unresolved, node/no-missing-import */ | ||
import { stringify as csvStringify } from 'csv-stringify/sync'; // https://github.com/adaltas/node-csv/issues/323 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These eslint issues seem like a known issue. I tried using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had a similar issue with |
||
/* eslint-enable */ | ||
import { domain, actorInfo, parseJSON } from '../util.js'; | ||
import { isAuthenticated } from '../session-auth.js'; | ||
import { lookupActorInfo, createFollowMessage, createUnfollowMessage, signAndSend, getInboxFromActorProfile } from '../activitypub.js'; | ||
|
@@ -91,6 +94,17 @@ router.get('/bookmarks.db', isAuthenticated, async (req, res) => { | |
res.download(filePath); | ||
}); | ||
|
||
router.get('/bookmarks.csv', isAuthenticated, async (req, res) => { | ||
const bookmarksDb = req.app.get('bookmarksDb'); | ||
const bookmarks = await bookmarksDb.getBookmarksForCSVExport(); | ||
const result = csvStringify(bookmarks); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. csv-stringify does also have an streaming API such that we could asynchronously stringify a row and then write that to the client. I didn't do that because this was simpler and we probably don't need to worry about memory ballooning during an export right? That would be so many bookmarks 😬 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I definitely am in favor of keeping things as simple as possible for now. If we start getting reports that memory limits are happening (especially on the Glitch containers), we can revisit this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this needs a tweak
I had some bookmarks in my database with null values in some columns (from a batch import), and they caused the CSV generation to go awry until I added this option to ensure that they were correctly handled. |
||
|
||
res.setHeader('Content-Type', 'text/csv'); | ||
res.setHeader('Content-Disposition', 'attachment; filename="bookmarks.csv"'); | ||
|
||
res.send(result); | ||
}); | ||
|
||
router.get('/activitypub.db', isAuthenticated, async (req, res) => { | ||
const filePath = `${DATA_PATH}/activitypub.db`; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a margin change here just because I thought there was a lot of space 🤷 . Can definitely remove if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem with this, the only comment I have is that to be consistent with the rest of the HTML code, switch over to
"
instead of'
(and throw a;
on the end of the style value)<p style="margin: 0;">
I think overall the web UI piece needs a lot more work, but this is fine from my POV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the web UI is definitely still rough. I have a couple talented friends who have offered to help out with this if they can find the time, but obviously we're all volunteering what we have to give here (and I'm still honored by how much time y'all have spent on this already). I'm fine with little stopgaps here and there with the understanding that we're probably going to blow up these templates and rebuild them.