connect using URI connection string support

[kukoo1]

As mentioned in #73.
Typescript doesn't allow spreading arguments into function microsoft/TypeScript#4130 and doesn't allow conditional super() call microsoft/TypeScript#945.

One way to achieve this is to instantiate Sequelize in a static factory function.
Old constructor must be removed to allow arguments to be exactly relayed to OriginSequelize constructor. Thus, this change will break backward compatability.

Developers currently using "new Sequelize(config)" have to change the code to "Sequelize.init(config)" or "Sequelize.init(URI, config)".

Should this be merged into next major version ?

[codecov-io]

Codecov Report

Merging #79 into master will increase coverage by 0.79%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   95.66%   96.46%   +0.79%     
==========================================
  Files          66       99      +33     
  Lines         761      904     +143     
  Branches      107      120      +13     
==========================================
+ Hits          728      872     +144     
  Misses         11       11              
+ Partials       22       21       -1

Impacted Files	Coverage Δ
lib/models/v3/Sequelize.ts	98.14% <100%> (+0.18%)	⬆️
lib/models/BaseSequelize.ts	95.12% <100%> (+4.21%)	⬆️
lib/annotations/hooks/BeforeCreate.ts	100% <0%> (ø)
lib/annotations/hooks/BeforeUpdate.ts	100% <0%> (ø)
lib/annotations/hooks/AfterUpsert.ts	100% <0%> (ø)
lib/annotations/hooks/AfterBulkCreate.ts	100% <0%> (ø)
...notations/hooks/BeforeFindAfterExpandIncludeAll.ts	100% <0%> (ø)
lib/annotations/hooks/ValidationFailed.ts	100% <0%> (ø)
lib/annotations/hooks/AfterBulkDestroy.ts	100% <0%> (ø)
lib/annotations/hooks/BeforeBulkCreate.ts	100% <0%> (ø)
... and 26 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1cfa77...1fdda44. Read the comment docs.

[RobinBuschmann]

Hey @kukoo1 thanks for contributing :)

Why is spreading necessary here, from your point of view?

Did you try to resolve this with overloading the constructor like:

export class Example {

  constructor(connection: string);
  constructor(options: Options);
  constructor(optionsOrConnection: Options|string) {
    /*...*/
  }
}

?

[kukoo1]

@RobinBuschmann I've tried this.

constructor(config: ISequelizeConfig, uri?: string) {
    // Sequelize constructor signature:
    //// new (database: string, username: string, password: string, options?: Options): Sequelize;
    //// new (database: string, username: string, options?: Options): Sequelize;
    //// new (uri: string, options?: Options): Sequelize;
   if (uri) {
      super(uri, BaseSequelize.prepareConfig(config));
   } else {
      super(
         (preparedConfig = BaseSequelize.prepareConfig(config), preparedConfig.name),
         preparedConfig.username,
         preparedConfig.password,
         preparedConfig
      );
   }
   this.init(config);
}

The compiler complains
A 'super' call must be the first statement in the constructor when a class contains initialized properties

So I tried solving with this.

constructor(config: ISequelizeConfig, uri?: string) {
   super(
      uri || (preparedConfig = BaseSequelize.prepareConfig(config), preparedConfig.name),
      uri ? BaseSequelize.prepareConfig(config) : preparedConfig.username,
      uri ? undefined : preparedConfig.password,
      uri ? undefined : preparedConfig
    );
    this.init(config);
}

Now it works with traditional connection options as an object.
But by using connection string, it won't work.
Because originalSequelize will recognize that super(uri, config, undefined, undefined) contains 4 arguments, so "uri" would be db name, "config" would be username, "undefined" would be password and so on.

Any ideas for this? :)

[RobinBuschmann]

@kukoo1 Thank you for your input :)

Or much simpler:

new Sequelize({uri: '...'})

constructor(config: ISequelizeConfig | ISequelizeUriConfig) {}

Since sequelize-typescript already uses one object literal for options instead of multiple parameters, I think this will be the most consistent approach. What do you think?

[chanlito]

hmm it's just a string, do we really need to make it object literal?

[RobinBuschmann]

@BruceHem I think it is most likely, that the configuration will look like this

new Sequelize({
  uri: '...',
  modelPaths: ['...']
})

But in addition, we could overload the constructor like:

  constructor(connection: string);
  constructor(options: ISequelizeConfig | ISequelizeUriConfig);
  constructor(optionsOrConnection: ISequelizeConfig | ISequelizeUriConfig | string) {
    /*...*/
  }

[kukoo1]

I found the root of this mess.
I've just looked into Sequelize code.
It accepts

new Sequelize({ ... options })
new Sequelize(URI, { ... options })
new Sequelize(database, username, password, { ... options })

So we could call super(uriOrOption) properly.

But the definition accepts only

new (uri: string, options?: Options): Sequelize;
new (database: string, username: string, options?: Options): Sequelize;
new (database: string, username: string, password: string, options?: Options): Sequelize;

So we've to open PR in DefinitelyTyped to add this beforehand

new (options: Options): Sequelize;

[kukoo1]

Now I'm getting into another trouble. I can't pass both URI and config at the same time (ISequelizeUriConfig).

Current constructor looks like this

constructor(configOrUri: ISequelizeConfig | ISequelizeUriConfig | string) {
   super(
      (typeof configOrUri === "string") ?
        configOrUri : // string
        configOrUri.uri || BaseSequelize.prepareConfig(configOrUri) //  ISequelizeUriConfig | ISequelizeConfig
   );
}

Sequelize checks arguments using this code

if (arguments.length === 1 && typeof database === 'object') {
   // new Sequelize({ ... options }) <-- in order THIS to WORKS, we must ONLY pass 1 argument !!
   // ...
} else if (arguments.length === 1 && typeof database === 'string' || arguments.length === 2 && typeof username === 'object') {
   // new Sequelize(URI, { ... options })
   // ...
}  else {  // <-- with this, URI string will NOT be WORKING !!
   // new Sequelize(database, username, password, { ... options })
   // ...
}

So we couldn't do this.

constructor(configOrUri: ISequelizeConfig | ISequelizeUriConfig | string) {
   // super({ ... options }) | super(URI, { ... options })
   super(
      (typeof configOrUri === "string") ?
         configOrUri : // string
         configOrUri.uri || BaseSequelize.prepareConfig(configOrUri), // ISequelizeUriConfig | ISequelizeConfig
      (configOrUri.uri) ?
         configOrUri : // ISequelizeUriConfig
         undefined // <-- THIS will BREAK arguments.length === 1
   );
}

There're 2 options.

1. ignore config when URI is passed (ISequelizeUriConfig will be a junk and we'll lose flexibility when using URI connection string)
2. open PR in Sequelize to add const argLength = arguments.filter(val => val !== undefined).length and use if (argLength === 1) instead.

[RobinBuschmann]

@kukoo1 Ahh ok, I see. I didn't get it in the first place.

From my understanding, typescript makes it impossible to use overloads in constructors in combination with inheritance in general. Cause it is very common to check arguments length. Or have we overseen something?

[kukoo1]

waiting for review ...
DefinitelyTyped/DefinitelyTyped#18896

[kukoo1]

@RobinBuschmann I committed the new one.

constructor(config: ISequelizeConfig | ISequelizeUriConfig | ISequelizeDbNameConfig);
constructor(uri: string);

I suggest to deprecate "name" property in options, and use "database" instead.
Because, in Sequelize documentation, "database" is used.
In order to use the same config when switching from ordinary sequelize, we should use "database".
What's your opinion ?

[RobinBuschmann]

Looks good to me. Deprecating "name" is reasonable as well. But instead of adding "@todo", you could write "@deprecated" - this will be noticed by many IDEs and therefore be notice the user that "name" is deprecated.

[RobinBuschmann]

Closes #73