sqj.js driver #894

daniel-lang · 2017-09-16T17:35:51Z

A driver for sql.js based on the AbstractSqliteDriver.
Two things I'm not quiet sure about yet:
Naming
is there a better way than calling it SqljsDriver?

Import/Export
Since the database in sql.js is never persisted the library supports an import/export mechanism. The problem with that is, that the database to import has to passed to the constructor in createDatabaseConnection. Therefore the only option I could come up with was use an option field (databaseForImport) which can be set to new Uint8Array(0) if nothing should be imported.
And with

connection.driver as any;
driver.export();

the database can be exported. Is there a nicer way for the import and maybe even the export?

Tested in the browser, but should also work in node.js

closes #822

pleerock · 2017-09-17T17:15:21Z

What if we implement following options:

Automatically manage database saving for user. When he connects we read from a provided database (from connection options). Each time when he saves/inserts/removes/updates something we save database (how to save it defined in connection options too)
Provide mechanizm for a user to manage saving by his own. For example:

connection.importDatabase(...); // or maybe call it loadDatabase or some better name

and

connection.exportDatabase(...); // or some better name

pleerock · 2017-09-17T17:31:27Z

connection.driver as any;
driver.export();

Too dirty for users. I would like to avoid adding sqljs-specific methods to connection, however using "any" or any other casting isnt an option. Alternative is to make things like in mongo - create SqljsEntityManager (like MongoEntityManager) and add there two new methods for export and import. Also we'll need to create getSqljsManager method in index.ts Actually I like it more then what Ive suggested before to add it in connection (including the fact how often sqljs will be used its a very pragmatic and elegant solution).

is there a better way than calling it SqljsDriver

Probably yes. Alternative is to name it SqlJSDriver, but Im not sure if its better.

Tested in the browser, but should also work in node.js

we need to include it in ormconfig.dist / ormconfig.travis to run then in nodejs as well. Also probably we'll need to include tests enabled only for this driver which cover this specific functionality (option to auto save or option to provided place)

Therefore the only option I could come up with was use an option field (databaseForImport) which can be set to new Uint8Array(0) if nothing should be imported.

Im not sure I completely understood this part.... But we need to make things flexible and easy for users. For example user needs to be able to provide simply path, like "/test/db.sql" and it should be automatically handled (for nodejs it may save database in file system and for browser it can save it in local storage). What do you think about it, is it implementable? This does not exclude option to provide utf8array too.

daniel-lang · 2017-09-18T06:06:12Z

Thanks for your feedback. I really like your ideas! I will see what I can do

pleerock · 2017-10-06T17:20:31Z

@daniel-lang any updates on this PR?

daniel-lang · 2017-10-06T17:24:02Z

I wasn't comfortable digging that deep into TypeORM, so I waited a bit to get more familiar with TypeORM. I hope, that I can update it this weekend or early next week so that it can be part of the 0.1 release

… look like This is still missing docs and tests

daniel-lang · 2017-10-09T19:04:01Z

@pleerock I push the most recent ideas.
As you suggested I added a special entity manager so functions like loadDatabase(), saveDatabase() and exportDatabase() can be called easily.
For autosave, I thought it might be a good idea to use an interval and leave it to the user to decide how often he wants his database saved. There are two options autoSave (enable it) and autoSaveInterval to configure it.

This is still missing:

docs
tests

pleerock · 2017-10-11T05:12:17Z

src/driver/sqljs/SqljsConnectionOptions.ts

+    /**
+     * True if file or localStorage should be updated automatically
+     */
+    readonly autoSave?: boolean;


Maybe its better to do boolean|number and if its a number, its an interval?

What would be happen, when it's set to true? Then the interval would have to be a default value, right?

I think we can make autoSave to boolean|Function instead of two parameters

Or maybe even save: boolean|string|Function where string is a location, what do you think about it?

pleerock · 2017-10-11T05:19:46Z

src/driver/sqljs/SqljsDriver.ts

+            }
+        }
+    }
+


make those methods protected too and put comments

I will add comments, but they are public because I have to call them from the entity manager

pleerock · 2017-10-11T05:20:14Z

src/driver/sqljs/SqljsQueryRunner.ts

+        this.driver = driver;
+        this.connection = driver.connection;
+    }
+


public methods are separate block, please try to keep following common code convention

daniel-lang · 2017-10-11T06:36:22Z

src/entity-manager/EntityManagerFactory.ts

@@ -16,6 +18,9 @@ export class EntityManagerFactory {
        if (connection.driver instanceof MongoDriver)
            return new MongoEntityManager(connection);

+        // if (connection.driver instanceof SqljsDriver)
+        //     return new SqljsEntityManager(connection, queryRunner);


I had to remove these lines becaus otherwise I get a TypeError: Object prototype may only be an Object or null: undefined error. Which, as far as I can tell, is because require('./EntityManager') inside SqljsEntityManager returns {}. But I don't really know why, the only information I found online was, that it could be a problem with circular dependency. But MongoEntityManager looks the same and works without problems

pleerock · 2017-10-11T15:03:50Z

.travis.yml

@@ -12,6 +12,7 @@ services:
  - docker

 before_script:
+  - npm install sql.js


why? we don't do same for other drivers. You just need to install sql.js and save into dependencies

Yes, you are right

…ft to fix circular dependency

daniel-lang · 2017-10-11T17:58:17Z

Tests for load, export and save exist now.

How do you want to do the autoSave feature?

Like it is right know with two options (enable and interval)
Like you suggested with one option (what would be the interval if it's just set to true?)
Like you originally suggested, where it would save after every query

pleerock · 2017-10-12T04:53:03Z

as for me interval does not make lot of sense. Database needs to be saved only after some modifications are applied on it, right? And usually modifications aren't so often, so saving database on that point kinda makes sense. Plus interval isn't reliable - in browser, what if interval is high (and making it low does not make sense because there are lot of reads and much less writes), what if user just closes the window and interval time isn't reached.

pleerock · 2017-10-12T04:53:42Z

Also we'll need to create a good facade/interface for users to easily configure how it gonna autosave (where?)

daniel-lang · 2017-10-12T05:18:27Z

Okay, I will rewrite the autosave part to save after insert, update and delete.

Also we'll need to create a good facade/interface for users to easily configure how it gonna autosave

What do you want the user to configure other than enable it and the location (both of them have an option)?

pleerock · 2017-10-12T08:38:56Z

What do you want the user to configure other than enable it and the location (both of them have an option)?

Do you mean location like "/test/db.sql" ? If yes, then can we implement it? Should be yes in nodejs, but what about browser? Shall we store it someone in the local storage? What about if user wants to save using his own method, how do he configure it?

daniel-lang · 2017-10-12T08:53:37Z

The way it's implemented right now is:

location option
- nodejs: specify a file location like "/test/db.sql"
- browser: specify the localstorage key to use (used like localstorage.setItem(location, database))
users can call SqljsEntityManager.exportDatabase() at any time and get the current database, but defining your one method that gets called on autosave is not support right now. But I like the idea

pleerock · 2017-10-12T09:23:29Z

Ah, nice, its implemented already. Then what are things left to be done?:

defining your one method that gets called on autosave

pleerock · 2017-10-13T15:48:50Z

Also, does it have any breaking change? If yes, then PR must be against 0.2.0 branch, if no, then we can merge it directly into master and release this change in 0.1.1

… not interval

pleerock · 2017-11-02T06:19:50Z

src/connection/Connection.ts

@@ -128,6 +129,13 @@ export class Connection {
        return this.manager as MongoEntityManager;
    }

+    get sqljsManager(): SqljsEntityManager {


missing comment block

pleerock · 2017-11-02T06:23:12Z

src/driver/sqljs/SqljsConnectionOptions.ts

+    readonly type: "sqljs";
+
+    /**
+     * The database definition and data to import


Don't forget dots after sentence 🙈

pleerock · 2017-11-02T06:24:10Z

src/driver/sqljs/SqljsConnectionOptions.ts

+/**
+ * Sqlite-specific connection options.
+ */
+export interface SqljsConnectionOptions extends BaseConnectionOptions {


I would improve all comments in this file - make them more concrete and simple for users understanding

pleerock · 2017-11-02T06:24:24Z

src/driver/sqljs/SqljsConnectionOptions.ts

+    readonly autoSaveCallback?: Function;
+
+    /**
+     * file path or local storage key to load and save database


Captal + dot

pleerock · 2017-11-02T06:25:36Z

src/driver/sqljs/SqljsConnectionOptions.ts

+    readonly database?: Uint8Array;
+
+    /**
+     * True if file or localStorage should be updated automatically


Bad comment

pleerock · 2017-11-02T09:18:48Z

src/driver/sqljs/SqljsDriver.ts

@@ -0,0 +1,188 @@
+import { AbstractSqliteDriver } from "../sqlite-abstract/AbstractSqliteDriver";


Please follow same code conventions everywhere - comment on each property, class, method, dots, etc.

pleerock · 2017-11-02T09:29:46Z

src/driver/sqljs/SqljsQueryRunner.ts

+                }
+
+                if (query.toUpperCase().match(/(COMMIT)/)) {
+                    this.driver.autoSave();


what if autoSave is an async operation? Callback provided by a user most probably should be a promise since it'll most probably save in the cloud / filesystem / database using async api

also this code looks a bit like a monkey patch. Isn't it better to do it after save method call? Plus after operations called by insert/update/delete query builders?

Yes, it's a bit of a monkey patch because I didn't want to save after every update/insert/delete, because they are always executed in a transaction, right? So I guess the best place would be commitTransaction().

what if autoSave is an async operation?

The question is, do we have to wait for the save operation to complete? What would change?

pleerock · 2017-11-02T09:34:52Z

src/entity-manager/SqljsEntityManager.ts

+ * that are unique to Sql.js
+ */
+export class SqljsEntityManager extends EntityManager {
+    private driver: SqljsDriver;


use code separation blocks

pleerock · 2017-11-02T21:10:31Z

src/connection/Connection.ts

+    /**
+     * Gets a sql.js specific Entity Manager that allows to perform special load and save operations
+     * 
+     * Available only in connection with the sqljs driver


doooots 😭

…itTransaction call

daniel-lang · 2017-11-03T20:28:33Z

The driver is tested in the browser

daniel-lang · 2017-11-03T20:30:29Z

test/functional/sqljs/auto-save.ts

+            }
+        ];
+
+        repository.createQueryBuilder("api").insert().into(Post).values(posts).execute();


The only problem left is the autoSave detection. Right now autoSave is called when a transaction is committed or insert(), update() or delete() in the query runner is called and no transaction is active.
But this will not detect bulk inserts like this one as they are executed using the query() method from query runner.
Any ideas?

maybe for now you simply call autoSave method of the driver in the case if dirver is sqlitejs right in the insert/update/delete query builders? Now Im not sure about better solution

I don't know if this is that much better than my original "monkey patch" where I checked if the query contains a INSERT, UPDATE or DELETE

pleerock · 2017-11-05T18:30:40Z

src/connection/Connection.ts

    /**
     * Creates an Entity Manager for the current connection with the help of the EntityManagerFactory.
     */
-    protected createEntityManager(queryRunner?: QueryRunner): EntityManager {
+    createEntityManager(queryRunner?: QueryRunner): EntityManager {


what is the purpose of this method being public? I don't really want to confuse users with both getEntityManager and createEntityManager. Plus there is a createQueryRunner which creates an entity as well...

I added this method to resolve a circlular dependency. And the EntityManager also uses the method...

pleerock · 2017-11-05T18:34:24Z

src/driver/sqljs/SqljsDriver.ts

-            const content = JSON.stringify(this.databaseConnection.export());
-            PlatformTools.getGlobalVariable().localStorage.setItem(path, content);
+            const database: Uint8Array = this.databaseConnection.export();
+            const databaseArray = [].slice.call(database);


what exact [].slice.call(database) does? copies array? Please add comment for dummies like me who dont know what this specific code does

pleerock · 2017-11-05T18:36:13Z

src/driver/sqljs/SqljsQueryRunner.ts

+     */
+    async commitTransaction(): Promise<void> {
+        await super.commitTransaction();
+        this.driver.autoSave();


did you miss comment where I told that autoSave most probably should be a promise?

No I didn't.
But I added back then: What would be the difference? I don't know if it would be a good idea to wait for the save operation to complete since it will slow down the driver.

it will be correct to wait a save operation to complete, otherwise users may face unexpected behaviour

Plus if there will be an error they will see it, it won't disappear in the air

I changed that, to wait for save

…oSave aswell

daniel-lang · 2017-11-07T19:26:49Z

src/connection/Connection.ts

+    /**
+     * Creates an Entity Manager for the current connection with the help of the EntityManagerFactory.
+     */
+    createEntityManager(queryRunner?: QueryRunner): EntityManager {


This is the only problem left....I don't know a way around making it public, because I need it in EntityManager to remove the dependency to EntityManagerFactory which again depends on EntityManager. I don't know why this error appeared now, after a added a second special EntityManager...

Okay, lets left it then

…s test as it is already included in other tests

pleerock · 2017-11-08T05:53:48Z

Are we done with this PR and it can be merged?

daniel-lang · 2017-11-08T07:40:20Z

I think so!

pleerock · 2017-11-09T11:09:52Z

Okay, I merged it. Can you please merge master brunch into next branch now? Next branch is 0.2.0.alpha and you can install it via npm i typeorm@next. Ive completely refactored persistence mechanizm (and still working on it). Conflicts shall be in "update"/"insert"/"delete" methods - they all removed in favour of query builder functionality. Logic of taking "last insert id" is moved into createGeneratedMap method

makamekm · 2017-11-10T11:03:34Z

Thank you very much! I'm gonna donate you.

daniel-lang added 2 commits September 16, 2017 19:27

added sql.js driver

cd8091a

fixed linting

fe06155

daniel-lang added 2 commits October 6, 2017 22:38

Merge remote-tracking branch 'upstream/master' into sqljs

9da7f23

driver(sqljs): Draft of how loading/saving/autosaving/exporting could…

0be2fc2

… look like This is still missing docs and tests

pleerock reviewed Oct 11, 2017

View reviewed changes

driver(sqljs): travis ci basic tests, refactoring, comments

1d6a757

daniel-lang commented Oct 11, 2017

View reviewed changes

driver(sqljs): fixed locking tests for sqljs

374deda

pleerock reviewed Oct 11, 2017

View reviewed changes

driver(sqljs): special tests for load, export and save functions, dra…

a1b57ee

…ft to fix circular dependency

daniel-lang added 4 commits October 23, 2017 12:19

Merge remote-tracking branch 'upstream/master' into sqljs

5b3b55a

driver(sqljs): changed driver to use work with callback on change and…

a72ab2b

… not interval

driver(sqljs): fixed failing tests

41c5713

driver(sqljs): added error handling

8363622

pleerock reviewed Nov 2, 2017

View reviewed changes

daniel-lang added 3 commits November 2, 2017 11:38

driver(sqljs): improved comments, added README and CHANGELOG note

fb5218c

Merge branch 'sqljs' of github.com:daniel-lang/typeorm into sqljs

d516822

Merge remote-tracking branch 'upstream/master' into sqljs

6ff089a

pleerock reviewed Nov 2, 2017

View reviewed changes

daniel-lang added 4 commits November 3, 2017 18:22

Merge remote-tracking branch 'upstream/master' into sqljs

47c81df

driver(sqljs): moved autosave call to insert, update, delete and comm…

7e109ef

…itTransaction call

driver(sqljs): fixed browser load and save mechanism

522623b

driver(sqljs): doooots... (comments)

44105c7

daniel-lang commented Nov 3, 2017

View reviewed changes

pleerock reviewed Nov 5, 2017

View reviewed changes

driver(sqljs): autoSave should be async, QueryBuilder has to call aut…

c83c622

…oSave aswell

daniel-lang commented Nov 7, 2017

View reviewed changes

driver(sqljs): added comment to explain array casting, removed option…

8f97ef5

…s test as it is already included in other tests

pleerock merged commit 4d7ea31 into typeorm:master Nov 9, 2017

daniel-lang deleted the sqljs branch December 19, 2017 10:06

		@@ -0,0 +1,188 @@
		import { AbstractSqliteDriver } from "../sqlite-abstract/AbstractSqliteDriver";

sqj.js driver #894

sqj.js driver #894

Conversation

daniel-lang commented Sep 16, 2017 • edited Loading

pleerock commented Sep 17, 2017

pleerock commented Sep 17, 2017

daniel-lang commented Sep 18, 2017

pleerock commented Oct 6, 2017

daniel-lang commented Oct 6, 2017

daniel-lang commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-lang commented Oct 11, 2017

pleerock commented Oct 12, 2017

pleerock commented Oct 12, 2017

daniel-lang commented Oct 12, 2017

pleerock commented Oct 12, 2017

daniel-lang commented Oct 12, 2017

pleerock commented Oct 12, 2017

pleerock commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-lang commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleerock Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleerock commented Nov 8, 2017

daniel-lang commented Nov 8, 2017

pleerock commented Nov 9, 2017

makamekm commented Nov 10, 2017 • edited Loading

daniel-lang commented Sep 16, 2017 •

edited

Loading

pleerock Nov 7, 2017 •

edited

Loading

makamekm commented Nov 10, 2017 •

edited

Loading