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

add database entries for new gists, update REPL URLs #2680

Merged
merged 5 commits into from
May 5, 2019

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented May 4, 2019

This builds on #2572:

  • Replaces the current /repl?gist=xyz123 URLs with /repl/xyz123
  • Replaces the current /repl?example=hello-world URLs with /repl/hello-world
  • Redirects /repl to /repl/hello-world
  • Moves /gist/create to /repl/create.json and /gist/xyz123 to /repl/xyz123.json
  • Falls back to the gist API if a given ID doesn't exist in the gists table of the database
  • Creates new gists (and users) in the database when doing so, so it only needs to happen once
  • Removes the gist link from the REPL toolbar
  • Turns the fork icon into a link to /repl/xyz123/fork (which would update the database and redirect the user to /repl/abc234) changed my mind about this. Instead, it inserts a history entry, so that the back button takes you to the parent
  • Handle relaxed logic
  • Possibly some other stuff

@@ -6,7 +6,7 @@ exports.up = DB => {
name character varying(255),
username character varying(255) not null,
avatar text,
github_token character varying(255) not null,
github_token character varying(255),
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be the only change to the production DB required

Copy link
Member

Choose a reason for hiding this comment

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

Will do

Copy link
Member

Choose a reason for hiding this comment

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

Done

@Rich-Harris Rich-Harris marked this pull request as ready for review May 5, 2019 00:08
@Rich-Harris
Copy link
Member Author

Alright, this is ready if anyone wants to take a look at it. The only other thing I thought about adding is a parent column in the gists table, in case we ever wanted to know which gist was forked to create a different gist. But I don't know how necessary that is.

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

I haven't run this locally, but have you tested logging in for the first time, despite a users row already existing for you via the Gist->Owner call?

https://github.com/sveltejs/svelte/blob/site/database-with-fallback/site/src/backend/auth.js#L108-L113

I'm 90% sure the on conflict still works for us in that case, but it did pop into my mind while reading.

@Rich-Harris
Copy link
Member Author

I did, yeah — it seems to work correctly

let k, cols=[], vals=[];
for (k in obj) {
cols.push(k);
vals.push(k === 'files' ? JSON.stringify(obj[k]) : obj[k]);
Copy link
Member

Choose a reason for hiding this comment

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

Did I write this? Can't tell if this file was moved or is new.

Looking at it, the JSON.stringify shouldn't be necessary... I think

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the JSON.stringify, it was erroring without it. I think it'll accept objects but not arrays (I changed the structure to files: [...] instead of files: {...} since we're no longer bound to the gist format, to reduce faffing

Copy link
Member

Choose a reason for hiding this comment

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

Ah, then okay. Arrays need explicit casting. We could also do { files } so that the input/output is automatic. Just have to remember to row.files.files, which may be too annoying.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i wondered about that. i reckon explicit casting is ok, since we probably won't need to add it in more places, and since we'll get a loud error if we do

Copy link
Member

Choose a reason for hiding this comment

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

I happy either way

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.

2 participants