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

Maybe steer people away from using the global db #44

Closed
LinusU opened this issue Nov 14, 2017 · 4 comments
Closed

Maybe steer people away from using the global db #44

LinusU opened this issue Nov 14, 2017 · 4 comments

Comments

@LinusU
Copy link
Contributor

LinusU commented Nov 14, 2017

Using globals variables are a bad practice for a number of reasons. In this case specifically it can have strange consequences if a library makes use of sqlite. If the application, or another library, also does that. They will most likely try and write to the same database instead of their separate ones which can make all sort of things go wrong.

A great first step I think would be to change the readme to point people to capture the output from db.open instead of throwing it away and using the global db.

How about something like the following?

import sqlite from 'sqlite'
import express from 'express'

const app = express()
const dbPromise = sqlite.open('database.sqlite').then(db => db.migrate())

app.get('/post/:id', async (req, res, next) => {
  try {
    const db = await dbPromise

    const [post, categories] = await Promise.all([
      db.get('SELECT * FROM Post WHERE id = ?', req.params.id),
      db.all('SELECT * FROM Category')
    ])

    res.render('post', { post, categories })
  } catch (err) {
    next(err)
  }
})

app.listen(/* ... */)

Long term I would love it if the global db could be dropped in the next major version...

@theogravity
Copy link
Collaborator

I don't see a problem with updating the readme - feel free to create a PR with the updated example. Another feature I've been using is instead of a file name, I use :memory: for an in-memory database (useful for testing)

@LinusU
Copy link
Contributor Author

LinusU commented Nov 15, 2017

PR sent 🚀

#45

@LinusU
Copy link
Contributor Author

LinusU commented Nov 15, 2017

Just realised that it could potentially be very nice to have an API that looks something like this:

import sqlite from 'sqlite'
import express from 'express'

const app = express()
const db = sqlite.open('database.sqlite', { migrate: true })

app.get('/post/:id', async (req, res, next) => {
  try {
    const [post, categories] = await Promise.all([
      db.get('SELECT * FROM Post WHERE id = ?', req.params.id),
      db.all('SELECT * FROM Category')
    ])

    res.render('post', { post, categories })
  } catch (err) {
    next(err)
  }
})

app.listen(/* ... */)

The Database implementation would look something like this:

const kDriver = Symbol('driver')

function migrate (driver) {
  return new Promise((resolve, reject) => {
    // ...
  })
}

class Database {
  constructor (filename, options = {}) {
    this[kDriver] = new Promise((resolve, reject) => {
      const driver = new sqlite3.Database(filename, (err) => {
        if (err) return reject(err)

        return (options.migrate ? migrate(driver) : driver)
      })
    })
  }

  run (sql) {
    return this[kDriver].then((driver) => {
      // ...
    })
  }

  // ...
}  

I would be very happy to make a PR if there is interest :)

@theogravity
Copy link
Collaborator

closing as there has not been activity in 3 years

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

No branches or pull requests

2 participants