Add csv export for bookmarks #118
Conversation
src/pages/admin/data.hbs
Outdated
| <p> | ||
| You can download your bookmarks sqlite db | ||
| <a href="/admin/bookmarks.db">here</a>. | ||
| <p style='margin: 0'> |
There was a problem hiding this comment.
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.
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.
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.
src/bookmarks-db.js
Outdated
| 'SELECT title,url,description,tags,created_at,updated_at from bookmarks', | ||
| ); | ||
| // These titles should be updated if the query changes above. | ||
| const columnTitles = {'title': 'title','url': 'url','description': 'description','tags': 'tags','created_at': 'created_at','updated_at': 'updated_at'}; |
There was a problem hiding this comment.
csv-stringify does have a way to specify titles when it stringifies, but I wanted to keep the titles local to this function so people didn't forget to update them if the query changed.
There was a problem hiding this comment.
I think we could get a little clever here and remove the need to update two things: something like this?
const columns = ['title', 'url', 'description', 'tags', 'created_at', 'updated_at'];
const query = `SELECT ${columns.join(',')} from bookmarks`;
const columnTitles = columns.reduce((o, c) => { o[c] = c; return o}, {})
There was a problem hiding this comment.
(that probably doesn't pass eslint, apologies, haha)
src/routes/admin.js
Outdated
| 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.
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.
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.
| @@ -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.
These eslint issues seem like a known issue. I tried using the csv package instead, but that didn't work either 😞
There was a problem hiding this comment.
I had a similar issue with string-strip-html here-- if anyone reading this knows why this happens, I'd love to know!
src/routes/admin.js
Outdated
| 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.
I think this needs a tweak
const result = csvStringify(bookmarks, {quoted: true});
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.
|
I guess a follow-on step from this would be to enable the reverse via the bulk import function, ie accept a file in CSV format that has been exported, for import. |
|
@andypiper good point, I just updated #23 to make it specifically targeting import from this export format. (We can open separate tickets over time for import from browser bookmark HTML files as well as export formats from Pinboard, Shaarli, etc) |
|
Thanks for the review @andypiper @ckolderup. I think this should be ready for review again when you have a chance. I did start looking into a csv import as well. The csv-parser package seemed like a good way to do that since it'd give us a parsed object that we could use to support other sources that output csv. |
This PR may close #86 unless there are additional desired formats.
Description
#86 describes a need to export the bookmarks into a friendlier format. This PR attempts to do that by creating a specific bookmarksDB function for that query and then using the csv-stringify package to send it to the client. This PR makes an additional small change to the admin/data template to export the formats as a list.
Testing
Alternatives Considered
I started out trying to just rely on sqlite to do the export. If you get in the console you can run:
and selects will come out as csv.
I was having an issue toggling those options on as part of the
db.all/exec/run. I think we could probably remove the csv-stringify dependency altogether if we could rely on this instead.FYI @ckolderup @andypiper