feat(sqlite): automatic path provision for 'options.storage' #11853
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11853 +/- ##
=======================================
Coverage 96.27% 96.27%
=======================================
Files 94 94
Lines 9209 9209
=======================================
Hits 8866 8866
Misses 343 343
Continue to review full report at Codecov.
|
papb
added
dialect: sqlite
papb
changed the title
|
Great work! |
papb
added
status: awaiting maintainer
and removed
status: awaiting response
| @@ -1,5 +1,7 @@ | |||
| 'use strict'; | |||
|
|
|||
| const path = require('path'); | |||
| const jetpack = require('fs-jetpack'); | |||
There was a problem hiding this comment.
Do we really need to use this new library, its unpacked size is ~300KB
There was a problem hiding this comment.
@sushantdhiman You're right. This package greatly simplifies the work with the file system, makes the code more concise. In addition, sequelize included it as devDependency. But I did not pay attention to its size. This in my opinion is not critical on the server side, but due to the fact that the file system is used by the sequelize only in ~ one place, I would like to do the following:
- Return the
fs-jetpackpackage back todevDependenciesand use it only for the convenience of writing tests - Write my own function(similar to
jetpack.dir()) to automatically provide the path forsqlite.options.storage
There was a problem hiding this comment.
It is only used for creating a path jetpack.dir(path.dirname(options.storage)), right? With Node 10 mkdirSync supports this https://nodejs.org/docs/latest-v10.x/api/fs.html#fs_fs_mkdirsync_path_options
There was a problem hiding this comment.
Awesome! Since sequelize 6.x will support node version 10 and more, we can actually use fs.mkdir(path, { recursive: true }). Can I do a pull request?
There was a problem hiding this comment.
That will be great @multum
There was a problem hiding this comment.
Oops, nice catch @sushantdhiman, sorry about that.
Pull Request check-list
Please make sure to review and check all of these items:
npm run testornpm run test-DIALECTpass with this change (including linting)?Closes #11663