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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SQLite support #2562

Merged
merged 12 commits into from Jan 31, 2019

Conversation

@pierreburgy
Copy link
Member

pierreburgy commented Dec 29, 2018

My PR is a:

  • 馃挜 Breaking change
  • 馃悰 Bug fix #issueNumber
  • 馃拝 Enhancement
  • 馃殌 New feature

Main update on the:

  • Admin
  • Documentation
  • Framework
  • Plugin

SQLite support has been highly demanded (#1562 and https://portal.productboard.com/strapi/c/35-support-sqlite-database). It will make setup much easier. Also, we will be able to offer a nice live demo using CodeSandbox 馃帀

Search is currently using LIKE syntax. If necessary, I can add FTS5 and triggers like explaining in this great article written by @jasonwyatt.

@derrickmehaffy

This comment has been minimized.

Copy link
Contributor

derrickmehaffy commented Dec 29, 2018

Nice one!

@Aurelsicoko

This comment has been minimized.

Copy link
Contributor

Aurelsicoko commented Dec 30, 2018

Nice job 馃憤

However, we should remember why we removed the SQLite support 8 months ago... (see #1025). It was probably due to a lack of time (cf #485) but we should be careful about this re-implementation.

Also, I was thinking about removing completely the database's step during the project's generation process. 95% of the time SQLite will perfectly match with the needs of the developer.

@derrickmehaffy

This comment has been minimized.

Copy link
Contributor

derrickmehaffy commented Dec 30, 2018

Also, I was thinking about removing completely the database's step during the project's generation process. 95% of the time SQLite will perfectly match with the needs of the developer.

Honestly that would probably be a good choice, let the users configure the database themselves of have another option with the CLI tool to configure one. I see a lot of questions related to migration process and creating a new app (short term). However I think merging the templates used would be best before this is done. (generate:api command, and admin generation)

@mnlbox

This comment has been minimized.

Copy link
Contributor

mnlbox commented Jan 1, 2019

@pierreburgy It's very good feature. Really thanks.
Two notes:

  1. Please move https://portal.productboard.com/strapi/c/35-support-sqlite-database to in progress.

  2. I think it's very handy and useful if we can develop in SQLite and deploy to production in other database. For example MongoDB or Postgres. Also if we have import/export feature (https://portal.productboard.com/strapi/c/23-import-export-data) we can develop on local with SQLite then export my systems as a file and import that in production server (for example with another database like MongoDB or Postgres)

  3. In future we can have an online API (system) designer maybe something like https://www.meteorkitchen.com/ that we can create API without install any external tools. Then we can export system as a file. Then we can import it trough Strapi command like CLI in local for more modification or directly in server for instant publish (for example with another database like MongoDB or Postgres). Also we can have an online system catalogue and download community api's something like: https://www.meteorkitchen.com/discover

pierreburgy added some commits Jan 3, 2019

@pierreburgy pierreburgy changed the title [WIP] Add SQLite support Add SQLite support Jan 3, 2019

@pierreburgy

This comment has been minimized.

Copy link
Member Author

pierreburgy commented Jan 3, 2019

This PR is now ready for tests 馃帀

Note 1: even in --dev mode, generated projects use the strapi-hook-bookshelf from npm. The problem is that, obviously, the connectivity.js file on npm does not contains the changes made in this PR. So, an error occurs when generating a new project. The work around is to generate a new project with Postgres and to replace the content of config/environments/development/database.json with:

{
  "defaultConnection": "default",
  "connections": {
    "default": {
      "connector": "strapi-hook-bookshelf",
      "settings": {
        "client": "sqlite",
         "fileName": ".tmp/data.db"
      },
      "options": {
        "useNullAsDefault": true
      }
    }
  }
}

Maybe @lauriejim has a work around to test it with the correct connectivity.js file.

Note 2: by default, the SQLite file is generated in .tmp (which is in the .gitignore of generated projects) with the name data.db. So the result is .tmp/data.db. Feel free to change it if necessary.

@pierreburgy

This comment has been minimized.

Copy link
Member Author

pierreburgy commented Jan 3, 2019

@mnlbox thank you for your comment!

  1. @Aurelsicoko could you move this feature to "In Progress", please?

  2. Exactly, this is why import/export feature is highly requested.

  3. This is interesting, but I think offering Strapi online would solve this problem in a more straightforward way.

@lauriejim lauriejim self-assigned this Jan 7, 2019

@lauriejim lauriejim requested review from lauriejim and Aurelsicoko and removed request for lauriejim Jan 7, 2019

@lauriejim lauriejim added this to In progress in 3.0.0-alpha.19 via automation Jan 7, 2019

@lauriejim lauriejim added this to the 3.0.0-alpha.19 milestone Jan 7, 2019

@@ -470,6 +470,7 @@ module.exports = function(strapi) {
try {
const connection = strapi.config.connections[definition.connection];
let columns = Object.keys(attributes).filter(attribute => ['string', 'text'].includes(attributes[attribute].type));
let indexes;

This comment has been minimized.

@Aurelsicoko

Aurelsicoko Jan 11, 2019

Contributor

Why? It seems that the variable isn't used anywhere. We should keep it in the scope of the if condition?

const directory = path.dirname(path.resolve(strapi.config.appPath, options.connection.filename));
if (!fs.existsSync(directory)){
fs.mkdirSync(directory);
if (!fs.existsSync(fileDirectory)){

This comment has been minimized.

@Aurelsicoko

Aurelsicoko Jan 11, 2019

Contributor

This function is deprecated (see https://nodejs.org/api/fs.html#fs_fs_exists_path_callback). You should use fs.access instead.

This comment has been minimized.

@soupette

soupette Jan 13, 2019

Member

I know that we often use such functions in the codebase to check whether or not a file or a folder does exactly exist like in here

try {
fs.accessSync(this.getDocumentationPath(apiName));
return true;
} catch (err) {
return false;
}

@Aurelsicoko I think that not only we @pierreburgy should change this function and put it the strapi-utils...

@lauriejim lauriejim removed this from the 3.0.0-alpha.19 milestone Jan 15, 2019

@lauriejim lauriejim added this to 馃暟Waiting for classification in 馃Cooking via automation Jan 15, 2019

@lauriejim lauriejim removed this from In progress in 3.0.0-alpha.19 Jan 15, 2019

@lauriejim lauriejim moved this from 馃暟Waiting for classification to 馃惪For next release in 馃Cooking Jan 15, 2019

lauriejim added some commits Jan 24, 2019

@lauriejim

This comment has been minimized.

Copy link
Member

lauriejim commented Jan 31, 2019

Relations with plugins model are broken. We have to fix it.

@lauriejim
Copy link
Member

lauriejim left a comment

LGTM!

Thank you @pierreburgy !

@lauriejim lauriejim added this to the 3.0.0-alpha.22 milestone Jan 31, 2019

lauriejim added some commits Jan 31, 2019

@lauriejim lauriejim merged commit 7d85075 into master Jan 31, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@lauriejim lauriejim deleted the sqlite-support branch Feb 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment