Remove dynamic require

[kentcdodds]

I'm using this module (through npm.im/got) for a serverless function and I'm getting this warning:

3:31:45 PM: WARNING in ../node_modules/keyv/src/index.js 18:14-40
3:31:45 PM: Critical dependency: the request of a dependency is an expression
3:31:45 PM:  @ ../node_modules/cacheable-request/src/index.js
3:31:45 PM:  @ ../node_modules/got/dist/source/core/index.js
3:31:45 PM:  @ ../node_modules/got/dist/source/create.js
3:31:45 PM:  @ ../node_modules/got/dist/source/index.js
3:31:45 PM:  @ ./api.js

It's not technically a problem, but it is an annoyance and I'd love to avoid the warning.

This simply removes the dynamic require for static and lazy requires. Just a refactor. No functionality change here.

[coveralls]

Coverage decreased (-2.4%) to 97.647% when pulling cc1649d on kentcdodds:patch-1 into 4cea2ce on lukechilds:master.

[Kikobeats]

Thanks for this PR 🙂 LGTM, just waiting for @lukechilds's opinion

[lukechilds]

Thanks for this @kentcdodds! Great solution, that dynamic require always annoyed me.

[Kikobeats]

I found a edge using webpack

 ERROR #98124  WEBPACK

Generating SSR bundle failed

Can't resolve '@keyv/sqlite' in '/Users/kikobeats/Projects/microlink/www/node_modules/keyv/src'

If you're trying to use a package make sure that '@keyv/sqlite' is installed. If you're trying to use a local file make sure that the path is correct.

File: node_modules/keyv/src/index.js

looks like webpack is trying to resolve require's even. It's a gatsby project 😢

[lukechilds]

Ahh yes, now you mention it I think I may have come across that before when trying to work around this.

I think the original warning is better than breaking builds so maybe we should revert this?

Alternatively we could add all official storage adapters as dependencies to Keyv, but I really don't think that's a good idea. That makes Keyv a very heavy dependency to add to any project and means installing it involves building SQLite from source, regardless of whether you use SQLite or not.

[Kikobeats]

on gatsby world, this was the workaround:

exports.onCreateWebpackConfig = ({ loaders, stage, actions }) => {
  if (stage === 'build-html') {
    actions.setWebpackConfig({
      module: {
        rules: [
          {
            test: /keyv/,
            use: loaders.null()
          }
        ]
      }
    })
  }

essentially because keyv is not used at all.

Agree with a reversion is a better approach

[lukechilds]

Ah good to see you can work around it.

But yeah, would be nice if we could make Keyv work a bit better with bundlers.

We could remove the magic that allows Keyv to resolve the storage adapter from the connection string and instead require people connect by passing in a storage adapter:

const KeyvRedis = require('@keyv/redis');
const Keyv = require('keyv');

const keyvRedis = new KeyvRedis('redis://user:pass@localhost:6379');
const keyv = new Keyv({ store: keyvRedis });

instead of the current:

const Keyv = require('keyv');
const keyv = new Keyv('redis://user:pass@localhost:6379');

This already works today, so we just need to remove allowing a connection string to be passed directly in to Keyv.

Alternatively we could still allow the connection string to be passed in to Keyv, but just require the user to manually register any storage adapters first:

const Keyv = require('keyv');
const KeyvRedis = require('@keyv/redis');

Keyv.registerStorageAdapter(KeyvMongo);

const keyv = new Keyv('redis://user:pass@localhost:6379');

Or we can revert the change in this PR and put up with the warnings from a dynamic require. (maybe we should do that immediately anyway to prevent build issues until we decide if we want to implement one of these solutions long term)

[Kikobeats]

@lukechilds I think it's a good opportunity for removing implicit keyv adapter string + drop Node v8 support (see #122) in a new major version