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

Proposal - require dependency directly from databases instead of use Store.use #122

Open
jacklam718 opened this issue Feb 20, 2018 · 20 comments

Comments

@jacklam718
Copy link

Proposal

require dependency directly from databases instead of use Store.use

For example:
aws = require('aws-sdk); instead of aws = Store.use('aws-sdk');
I propose this idea because I have some issues.

Background

I using AWS Lambda with serverless and serverless-webpack
I want to use eventstore with dynamodb from lambda but I got some errors. btw, I've installed aws-sdk and works on my local.

1. Implementation for db \"dynamodb\" does not exist!.

my code:

import eventstore from 'eventstore';
eventstore({ type: 'dynamodb' });

then I tried pass the DynamoDB class to eventstore, but I got another error Cannot find module 'aws-sdk' from parent
my code:

import eventstore from 'eventstore';
import DynamoDB from 'eventstore/lib/databases/dynamodb';
eventstore({ type: DynamoDB });

Some of my thoughts

About the issue 1 and 2. I think it's because it cannot find modules via require after webpack bundled.
https://github.com/adrai/node-eventstore/blob/master/index.js#L42
https://github.com/adrai/node-eventstore/blob/master/lib/databases/dynamodb.js#L2
https://github.com/adrai/node-eventstore/blob/master/lib/base.js#L172

So, that's why I propose this idea.

@jacklam718
Copy link
Author

I could submit pull request if you agree my proposal.

@adrai
Copy link
Contributor

adrai commented Feb 20, 2018

Unfortunately this module was not designed to be used with webpack...
the strategy here is to have the database "drivers" in control of the user.
Another idea could be to have an "own" postinstall script that installs the dependency in the appropriate directory...

@jacklam718
Copy link
Author

Ok. Thanks for the quick reply.

@jacklam718
Copy link
Author

jacklam718 commented Feb 20, 2018

But why not just require('aws-sdk') from dynamodb.js directly? It can solve this issue.
And, I think serverless and serverless-webpack quite famous maybe other people will have same issue...

@adrai
Copy link
Contributor

adrai commented Feb 20, 2018

because of this: https://github.com/adrai/node-eventstore/blob/master/lib/base.js#L174
I've seen this even for prod environments

@jacklam718
Copy link
Author

jacklam718 commented Feb 20, 2018

What about do something like this?

dynamodb.js
var util = require('util');
var Store = require('../base');
var _ = require('lodash');
var async = require('async');
var dbg = require('debug');
try {
  var aws = require(aws-sdk);
} catch (e) {
  // workaround when `npm link`'ed for development
  var aws = prequire(aws-sdk);
}

@jacklam718
Copy link
Author

jacklam718 commented Feb 20, 2018

Because I don't want other people like me. having same issue and spend few hours to investigate 😛
Really appreciate your quick reply 😀

@adrai
Copy link
Contributor

adrai commented Feb 20, 2018

For sure this will work, but than every implementation needs to do this... that's why we have moved this to a dedicated function.

If that is a concern of several people, we can do that.

The problem is that all cqrs relevant modules are constructed that way.

I also do not understand why it's necessary to use webpack in the backend.

In the mean time I would suggest you to use your fork (if you really need webpack)

@jacklam718
Copy link
Author

Because AWS Lambda doesn't support ES6. And, serverless-webpack is official plugin 😶

@adrai
Copy link
Contributor

adrai commented Feb 20, 2018

I know... https://twitter.com/adrirai/status/965700927670931456
but in my opinion... (really just my opinion) I would never transpile something in backend just because it's new and everyone is doing it in frontend... just wait until the real runtime is ready and available (then it's even faster and you need less code)

@jacklam718
Copy link
Author

btw, finally I solved the issue by using babel because babel only do transpile.

@eugenio79
Copy link

I had a similar problem with eventstore and it got solved by using webpack-node-externals.

@dbartholomae
Copy link

@adrai Our main reason for using webpack is that we write our code in TypeScript. Having strongly typed code is even more useful in the backend than in the frontend. Basically we have four alternatives now:

  1. Try out a different transpiler to get TypeScript working - which might not actually solve the problem.
  2. Find a way to get this library to work with webpack - would only make sense if this is also in your interest
  3. Use a different library for CQRS - would be a shame, haven't found another good one yet
  4. Drop TypeScript and typesafety - this for us is definitely out, type safety was a giant win for our code base.

What's your opinion?

@adrai
Copy link
Contributor

adrai commented Jun 3, 2019

@dbartholomae My honest opinion is: I have a "certain" opinion about TypeScript... but this is another story...
I will personally not maintain any typescript types or files here...
but as said before... this is just my opinion...
What do you think @nanov?

@dbartholomae
Copy link

Don't worry, not talking about adding typings in here ;)
Just about making it possible to use the library with a transpiler :)

@nanov
Copy link
Contributor

nanov commented Jun 4, 2019

Just my two cents, I don‘t really get the problem to use the library with your favorite transpirier, be it rollup, webpack or parcel. All of them have settings and options which allow this, is the dynamic import the only issue here?

@dbartholomae
Copy link

So far we have multiple days just trying to set this up with webpack and don't see a light at the end of the tunnel yet :-/

@nanov
Copy link
Contributor

nanov commented Jun 4, 2019

If you are not set on webpack, I would definitely give parcel a shot. If you supply some repo, I could take a look.

The thing is that, I guess your problems come from the dynamic imports, as such i‘m not sure that the proposal from this issue will solve them.

@nanov
Copy link
Contributor

nanov commented Jun 10, 2019

I just had an idea, which might work for you, if we provide the ability to supply the library with the options so the implementation wont have to require it by itself ( dynamically ), will that work for you?

@dbartholomae
Copy link

We are currently in the works of getting the first prototype live. As soon as we have it (I assume next week) we will share the code base and some ideas for how it could be improved for our usage case :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants