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

Issues with multi-database libs: sequelize #345

Closed
konsumer opened this issue Apr 4, 2019 · 19 comments · Fixed by #407
Closed

Issues with multi-database libs: sequelize #345

konsumer opened this issue Apr 4, 2019 · 19 comments · Fixed by #407
Labels
dynamic require package issue priority Important issue or pull request to fast-track

Comments

@konsumer
Copy link

konsumer commented Apr 4, 2019

I am having trouble using any multi-database wrappers (tried sequelize and any-db) with ncc.

Let's take, for example, a URI like sqlite:///tmp/demo.sqlite, which works fine locally in an express service. If I compile with ncc, it runs fine if the output is in the same dir (where sqlite3 is installed.)

Here is me troubleshooting with now.sh.

Here is a demo-repo that illustrates the problem. If I deploy or build locally but move the folder out of the project that has sqlite3 installed, it exhibits the problems above.

To reproduce:

git clone https://github.com/konsumer/demo-db.git
cd demo-db
npm i

ncc build node/index.js                                                              # builds fine
DATABASE_URI="sqlite:///tmp/demo.sqlite" node dist/index.js # runs fine

mv dist ~/Desktop
cd ~/Desktop/dist
DATABASE_URI="sqlite:///tmp/demo.sqlite" node index.js

/home/konsumer/Desktop/dist/index.js:28596
        throw new Error(`Please install ${moduleName} package manually`);
        ^

Error: Please install sqlite3 package manually
    at ConnectionManager._loadDialectModule (/home/konsumer/Desktop/dist/index.js:28596:15)
    at new ConnectionManager (/home/konsumer/Desktop/dist/index.js:21768:21)
    at new SqliteDialect (/home/konsumer/Desktop/dist/index.js:37324:30)
    at new Sequelize (/home/konsumer/Desktop/dist/index.js:22307:20)
    at Object.<anonymous> (/home/konsumer/Desktop/dist/index.js:72980:19)
    at __webpack_require__ (/home/konsumer/Desktop/dist/index.js:22:30)
    at startup (/home/konsumer/Desktop/dist/index.js:36:19)
    at /home/konsumer/Desktop/dist/index.js:42:18
    at Object.<anonymous> (/home/konsumer/Desktop/dist/index.js:45:10)
    at Module._compile (internal/modules/cjs/loader.js:805:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:816:10)
    at Module.load (internal/modules/cjs/loader.js:672:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:612:12)
    at Function.Module._load (internal/modules/cjs/loader.js:604:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:868:12)
    at internal/main/run_main_module.js:21:11

Might be related to #74

@konsumer
Copy link
Author

konsumer commented Apr 4, 2019

Without using a multi-db manager, it seems to work great:

const { Database } = require('sqlite3')

const db = new Database(process.env.DATABASE_URI.replace(/^sqlite:\/\//, ''))

const query = q => new Promise((resolve, reject) => db.all(q, (err, rows) => err ? reject(err) : resolve(rows)))

const run = async () => {
  const r = await query('SELECT 1+1 AS result')
  console.log(r[0].result)
}
run()

and creates a atomic dist folder that looks like this:

├── index.js
├── lib
│   └── binding
│       └── node-v67-linux-x64
│           └── node_sqlite3.node
├── node-pre-gyp
│   ├── appveyor.yml
│   ├── bin
│   │   ├── node-pre-gyp
│   │   └── node-pre-gyp.cmd
│   ├── CHANGELOG.md
│   ├── contributing.md
│   ├── lib
│   │   └── util
│   │       ├── abi_crosswalk.json
│   │       └── nw-pre-gyp
│   │           ├── index.html
│   │           └── package.json
│   ├── LICENSE
│   ├── package.json
│   └── README.md
├── nw-pre-gyp
│   ├── index.html
│   └── package.json
└── package.json

While this is great, I'd really like the flexibility of using multiple databases.

@konsumer
Copy link
Author

konsumer commented Apr 5, 2019

Here is that code running on now.

Also tried knex, which seemed to work fine, so I might end up just using that.

@styfle
Copy link
Member

styfle commented Apr 5, 2019

Hi @konsumer thanks for the bug report!

There is a lot here so maybe we can isolate it to one specific issue.

Without using a multi-db manager, it seems to work great:

What is a multi-db manager?

You might also be interested in these related issues to DBs:

https://github.com/zeit/now-builders/issues/331#issuecomment-479034684

https://github.com/zeit/now-builders/issues/335#issuecomment-480009057

@konsumer
Copy link
Author

konsumer commented Apr 5, 2019

What is a multi-db manager?

I tested with sequelize and any-db. What I mean is a lib that lets you use multiple databases with the same code. I would call it an ORM, but any-db isn't an ORM, just a lib that lets you talk to multiple SQL databases with the same code. For further testing I tried knex, and it seemed to work ok, so for my use-case I might just use that.

You might also be interested in these related issues to DBs:

Thanks!

Link 1: Since the code works fine compiled in ncc, directly, as long as it's in the same dir that has node_modules, but not if not, I think it's a problem with ncc or how now is bundling node_modules. The linked issue is about setting up other things (AWS credentials for RDS) but my example literally just opens a sqlite file in /tmp and does SELECT 1+1. no other settings are needed. The examples also verify that the path tho the sqlite file is correctly inserted into env vars as /tmp/demo.sqlite

Link 2: This is getting closer to my issue, I think, in that the mongodb-person is saying "mark it as an external module" which ncc is definitely doing automatically with sqlite3 (again, compiled code works fine in same project that has sqlite installed in node_modules, but not if you take the compiled dir out of that folder.) Again, this points to now's method of dealing with node_modules or how ncc bundles the deps. Since someone from the now team said to report it here, I am doing that.

@styfle
Copy link
Member

styfle commented Apr 5, 2019

Have you tried adding const sqlite = require('sqlite3') before sequelize?
Or tried using config.bundle = false?

See my comment here: https://github.com/zeit/now-builders/issues/331#issuecomment-478679523

@konsumer
Copy link
Author

konsumer commented Apr 5, 2019

Have you tried adding const sqlite = require('sqlite3') before sequelize?

Yeh, this was my first idea, to prime the dependency-tree. I get module initialization error: Error with no clear reason why.

  • Here is that running on now.
  • Here is the code.

Or tried using config.bundle = false?

I combined both suggestions, and got the same error.

  • Here is both suggestions running on now.
  • Here is the code.

@styfle
Copy link
Member

styfle commented Apr 5, 2019

config.bundle only works with the @now/node-server builder.

Also, you might try creating separate now.json files in each directory instead of one at the top level of the monorepo. Maybe the bug only shows up with monorepos 🤔

@konsumer
Copy link
Author

konsumer commented Apr 5, 2019

config.bundle only works with the @now/node-server builder.

That works. Awesome!

  • Here is that running on now.
  • Here is the code.

If you want to test out more ideas, feel free to use my demo repo as it has a few permutations that illustrate the problem.

Also, you might try creating separate now.json files in each directory instead of one at the top level of the monorepo. Maybe the bug only shows up with monorepos

A big part of my reason for wanting to use now in my situation, is that it simplifies a single host with multiple lambda mount-points. If I can't have that sort of setup, I see little advantage over serverless. I mean, I can even do that with serverless, it just takes a little more configuration. up also does separate individual endpoints simply, and doesn't have this problem.

This node-server solution seems like an OK middleground, even though I'm not getting the improved bundle-sizes, or the other advantages of @now/node, but I can mix the one service that needs a database in with others that don't need it, so I can totally live with that. Thanks!

@guybedford
Copy link
Contributor

Just had a quick look at this one, and it seems the untraceable dynamic require is in https://github.com/sequelize/sequelize/blob/7445423b9effc212e125d76d58e02713e808ffc6/lib/dialects/abstract/connection-manager.js#L69.

Configuring --external sqlite should also get this to work here with ncc, although I'm not sure how that same workflow would look in Now.

@konsumer
Copy link
Author

konsumer commented Apr 10, 2019

@guybedford thanks for taking a look. That's a great tip for working around dynamic modules in ncc. I ended up just using knex on now, which seems to work great with @now/node@canary builder.

@styfle styfle changed the title Issues with multi-database libs. Issues with multi-database libs: sequelize Apr 10, 2019
@styfle styfle added package issue priority Important issue or pull request to fast-track labels Apr 23, 2019
@styfle
Copy link
Member

styfle commented Apr 23, 2019

@guybedford Can we implement a workaround to get sequelize working?

There are other reports of a similar issue when using sequalize and pg here: https://github.com/zeit/now-builders/issues/331

@guybedford
Copy link
Contributor

The workaround here is to pass the dialect to the Sequelize initialization itself.

So instead of having:

let db = new Sequelize({
  dialect: 'pg'
});

You would do:

let db = new Sequelize({
  dialect: 'pg',
  dialectModule: require('pg')
});

where that then avoids the internal dynamic require which would just do require(config.dialect), which is the thing we can't analyze.

@zdne
Copy link

zdne commented Nov 26, 2019

I can confirm this is still issue with the current version and sequelize v5. Adding dialectModule: require('pg') as @guybedford suggested helps.

@thisjeremiah
Copy link

Similarly, I can confirm that adding dialectModule: require('sqlite3') worked for me.

@aecorredor
Copy link

I'm having the same issue, but my dialect has always been postgres, not pg. I'm guessing they are the same. Even with require('pg'), I still get the following error instead: Cannot find module 'pg-native'

@konsumer
Copy link
Author

konsumer commented Jan 8, 2020

@aecorredor you should be able to do it with this:

const db = new Sequelize({
  dialect: 'postgres',
  dialectModule: require('pg-native')
});

@focus1691
Copy link

dialectModule: require('pg-native')
Didn't fix it for me

And dialectModule: require('pg') is giving me many errors such as ModuleNotFoundError: Module not found: Error: Can't resolve 'dns'

@piociccale
Copy link

need to import also library

@jmsunseri
Copy link

jmsunseri commented Jan 10, 2022

Just had a quick look at this one, and it seems the untraceable dynamic require is in https://github.com/sequelize/sequelize/blob/7445423b9effc212e125d76d58e02713e808ffc6/lib/dialects/abstract/connection-manager.js#L69.

Configuring --external sqlite should also get this to work here with ncc, although I'm not sure how that same workflow would look in Now.

what does this mean? i tried adding

vite: {
			build: {
				rollupOptions: {
					external: ['pg-native']
				}
			}
		}

or

vite: {
			build: {
				rollupOptions: {
					external: ['pg']
				}
			}
		}

neither seems to fix the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dynamic require package issue priority Important issue or pull request to fast-track
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants