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

Import a single Gist by ID #76

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Import a single Gist by ID #76

merged 2 commits into from
Apr 14, 2022

Conversation

jazcarate
Copy link
Collaborator

There are 2 "discussion points":

Limitations

  1. Adding a gist with files larger than 10mb
  2. Adding a gist with more than 300 files

The 1 is easy enough, as you can fetch the raw_url direct.
The 2 is a bit tricky, as one should clone the git repo. I was not about to make the server clone a repo to get the 301th file 😅 so I opted to simply throw. We can improve on it later 🤷

Gists to test these limits:

  1. Big file: https://gist.github.com/jazcarate/50bb04ad4904dc42dce512c10e9e0557
  2. Many files: https://gist.github.com/jazcarate/bf8b6ac8c82f09946e350814aa3c84a1

Creation date

I opted to import the creation date of a gist, but not the updated_at. It is a bit weird that you can import an old gist, and not see it in the /mine, but I felt it was more convenient to keep the created_at. We can make the creation date be the imported date. I don't have a good reason for either one 😸

Pending

A UI. I wanted to push this first part, discuss the endpoint, then make a bulk import (maybe a queue? so we don't overload the server. Or even push the fetching to the frontend 🤔)

@vercel
Copy link

vercel bot commented Apr 8, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @MaxLeiter on Vercel.

@MaxLeiter first needs to authorize it.

@jazcarate jazcarate changed the title lint Import a single Gist by ID Apr 8, 2022
MaxLeiter
MaxLeiter previously requested changes Apr 8, 2022
View reviewed changes
server/src/lib/gist/fetch.ts Outdated Show resolved Hide resolved
server/src/lib/gist/fetch.ts Outdated Show resolved Hide resolved
server/src/routes/posts.ts Show resolved Hide resolved
server/src/lib/gist/__tests__/index.ts Show resolved Hide resolved
@MaxLeiter
Copy link
Owner

MaxLeiter commented Apr 9, 2022 •
edited
Loading

Thanks for this!

What's the problem with /mine and created_at? I think it makes sense to set created_at to the original Gist date and updated_at to the time of the import. Then we can change /mine to sort by updatedAt || createdAt and /mine should be accurate, right?

Re your discussion points, I'm fine with erroring on both cases as long as we make it clear to the user why: Sorry, we don't currently support <uploading 10MB files | 300 files>

@jazcarate jazcarate requested a review from MaxLeiter April 9, 2022 07:49
@jazcarate
Copy link
Collaborator Author

What's the problem with /mine and created_at?

Not a problem, so much as weird UX. Say you have an old Gist, and you created 10 Drift Posts.

  1. You go to /mine you see your 10 posts
  2. You import the old gist
  3. You go back to /mine and nothing changed (because the Gist is older than the 10 posts)

Sorting by createdAt does fix this UX, but then your mine would be a bit disorganized. Every update to a drift would reorder the /mine.

Given that importing a gist should be much less common than updating an existing post; I left the createdAt as the Gist's creation date. But I wanted to point out the scenario ☝️. Maybe there is a better solution, like having an aditional DriftCreationAt that is used to sort /mine, but it is always the new Date() regardless of a creation or an import🤷.
TBH, I'd leave this as is, and see if anyone complains in the future when importing Gists 😸

@MaxLeiter MaxLeiter mentioned this pull request Apr 9, 2022
Open
@MaxLeiter
Copy link
Owner

Sorting by createdAt does fix this UX, but then your mine would be a bit disorganized. Every update to a drift would reorder the /mine.

Do you mean sorting by updatedAt? I think it's fine if modifying the visibility changes it in /mine.

Alternatively, we add a source string (open to other names) to the Post model that can for now be "drift" | "gist". In the future this could be nice if someone uses a tool or API key to create a post.

@jazcarate
Copy link
Collaborator Author

Do you mean sorting by updatedAt?
Yeah 😅

I wouldn't add a source yet. And I'll work on a restructure of the server/ after this get merged :)

@jazcarate jazcarate dismissed MaxLeiter’s stale review April 14, 2022 21:41

Dunno why it is still flagging as "need changes". I though I resolved all requests 😕

@MaxLeiter MaxLeiter merged commit c0566ef into MaxLeiter:main Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxLeiter MaxLeiter MaxLeiter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jazcarate @MaxLeiter