Loosen semver ranges on 3rd-party typings #166

schmod · 2017-10-13T13:11:34Z

Fixes #165, and possibly some other issues that may result from this project using a slightly older version of @types/sequelize and typescript.

Due to one of these updates, the behavior of getModels() had to be slightly modified to conform with its type signature. This should not be a breaking change for anybody.

Also fixed up a few tests that were using strange (and seemingly undocumented) variations of the where: operator.

schmod · 2017-10-13T13:12:29Z

test/specs/instance.spec.ts

      User
-        .findOne({where: ['asdasdasd']})
+        .findOne<User>({where: <any>{ bad: 'asdasdasd' }})


Incidentally, the newer Sequelize typings actually turn bad where clauses into a compiler error, making this sort of error a bit harder to trigger.

schmod · 2017-10-13T13:14:47Z

test/specs/instance.spec.ts

-              username: 'cuss'
-            })
+            where: {
+              [sequelize.Op ? sequelize.Op.and : '$and']: { username: 'cuss' }


Awkward workaround to simultaneously support Sequelize v3 and v4.

@types/sequelize for v4 allows some of the legacy operators, but not all of them (there might be a reason for it, but I can't find one). I'll send them a separate PR to fix that.

In #157 the project decided to drop v3 support - however since that hasn't happened yet (in code), I'm not sure what the best course of action for this code is. For now what you have is probably good, since the codebase currently tries to support both - but when we drop v3 we can get rid of the awkward workaround.

Yep, but as long as we've got tests targeting v3....

codecov-io · 2017-10-13T13:17:24Z

Codecov Report

Merging #166 into master will decrease coverage by 0.2%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master     #166      +/-   ##
==========================================
- Coverage   96.84%   96.63%   -0.21%     
==========================================
  Files         100      100              
  Lines         918      921       +3     
  Branches      124      125       +1     
==========================================
+ Hits          889      890       +1     
- Misses          9       10       +1     
- Partials       20       21       +1

Impacted Files	Coverage Δ
lib/annotations/Column.ts	`91.66% <100%> (ø)`	⬆️
lib/services/models.ts	`90.51% <75%> (-1.52%)`	⬇️

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 ca40bbd...ba1880e. Read the comment docs.

snewell92 · 2017-10-13T13:28:16Z

lib/services/models.ts


    return arg.reduce((models: any[], dir) => {

+      if (typeof dir !== 'string') {


@RobinBuschmann do we have tests on this method? (I don't think I saw any in model.spec.ts?) This would be good to have some tests. (array with Model and String elems / only string elems / only Model elems / empty array / null / object).

@snewell92 No, we don't have any for this method only. @schmod Can you add some tests as a part of this PR? This would also increase coverage :)

snewell92 · 2017-10-13T13:37:40Z

@schmod Glad to have a new first time PR 😺I think it's golden, just not sure if we could add tests to increase coverage or not (hence ping to Robin), but Codecov has been known to be finicky about small things ¯\_(ツ)_/¯

@RobinBuschmann could you look over this when you can, just to look over the change in getModels - then I believe this can be merged.

RobinBuschmann · 2017-10-14T11:35:30Z

package.json

-    "@types/node": "6.0.41",
-    "@types/reflect-metadata": "0.0.4",
-    "@types/sequelize": "4.0.73",
+    "@types/bluebird": "^3.5.15",


Let us keep the exact versions here. Here is a discussion about it: #142

RobinBuschmann · 2017-10-14T11:50:10Z

Maybe we can get rid of setting the bluebird typings explicitly as a dependency, cause they are required by the sequelize typings anyway. What do you think, @snewell92 @schmod ?

snewell92 · 2017-10-16T13:22:37Z

If we drop bluebird (and remove its use from Model.d.ts and BaseModel.d.ts), will that allows users to use bluebird in their applications and the promises that sequelize-typescript / sequelize returns will be promises? If so, that sounds great! I'm always down for removing code and dependencies if the library/app can still do the same thing without it.

schmod · 2017-10-16T14:13:12Z

@snewell92 Sequelize already uses Bluebird promises "out of the box", and the typings accurately reflect this. You should theoretically be able to do this already.

Unfortunately, there have been "semver-minor" changes to Bluebird's typings that appear to be incompatible with each other, which is why this does not currently appear to work in many cases.

Getting rid of the Bluebird typings probably won't be practical, unless we can explicitly import them from @types/sequelize (which feels a bit hacky).

If we're maintaining a hard lock on a specific version of @types/sequelize, we might as well just make the version specifier of @types/bluebird in our package.json match theirs. I think that should resolve a significant portion of the compatibility issues that we're seeing.

Given the recurrence of these issues, I think our docs need to explain this project's relationship with @types/sequelize, and advise users to import { Sequelize } from 'sequelize-typescript instead of loading the upstream sequelize types directly (given the likelihood that they'll import a different version). We may be able to enforce this via a peerDependency or optionalDependency.

snewell92 · 2017-10-16T14:47:53Z

we might as well just make the version specifier of @types/bluebird in our package.json match theirs

I think it makes sense to do what @types/sequelize is doing with regards to typing versions, especially since we depend on a specific version of it (due to incompatibility issues).

We may be able to enforce this via a peerDependency

A peer dependency! That sounds very reasonable. Now that I think about it... yes. That is exactly the relationship sequelize-typescript expects with sequelize (since it doesn't have a dep on sequelize, but won't work without it).

RobinBuschmann · 2017-10-16T18:24:25Z

Regarding bluebird: Hmm, I probably miss the point here or I was not clear enough. The bluebird typings will be installed anyway, cause they are a dependency of the sequelize typings. So there is nothing left to do... or maybe I don't understand what are you going to say? 😱

schmod · 2017-10-19T20:15:51Z

The bluebird typings will be installed anyway

I don't think this is a great assumption to make. It might not always be true, and contradicts the conventions typically used by Node.JS projects -- I can't think of any projects that require() a 3rd-party module without listing it in their package.json file.

For example, this is a perfectly valid tree that NPM (or some other package manager) could produce:

.
└── node_modules
    ├── bluebird
    ├── @types
    │   └── sequelize
    │       └── node_modules
    │           └── @types/bluebird
    └── sequelize-typescript

In this example, the require.resolve() algorithm would be unable to locate @types/bluebird from the root of sequelize-typescript.

RobinBuschmann · 2017-10-20T06:30:35Z

I can think of two scenarios, where this would happen: on an old node version or if the bluebird typings are already installed. Is there another one ?

Anyway, you're absolutely right, an explicit solution is better.

However I think we can omit the bluebird typings. Because the sequelize typings exports them. So that sequelize-typescript could use promises like this `import {Promise} from 'sequelize'; Do I miss something?

snewell92 · 2017-11-14T18:15:48Z

Just revisiting - I still really like @schmod 's suggestion of a peer dep to sequelize, and matching bluebird typings to what @types/sequelize lists, and of course matching bluebird to what sequelize lists too, so that when we bump a sequelize version, we then must match bluebird and its typings for the appropriate version of sequelize we're bumping up too.

This will ensure correctness in our types.

@RobinBuschmann I think your two scenarios fail to capture the complexity of npm's package system. There are a lot of different scenarios:

No bluebird installed at the root
bluebird installed at the root
a. bluebird at root matches our version
b. bluebird at root does not match

Then consider, in the dependency tree, since sequelize installs bluebird too, if our bluebird and its types match sequelize's, and if either of those (sequelize or our's) match the root import. Not to mention if there are other packages that may or may not wrap, call into, or use sequelize/sequelize-typescript (like feathers-sequelize for example)

Thus we reach a combinatorial explosion of cases. (I think)

The typescript compiler can get the appropriate types at each level, but the problem is that if bluebird introduces some breaking change, and if sequelize or sequelize-typescript call into their own copy of bluebird down the dep tree - then typescript might report something that doesn't actually exist (since at the root, bluebird is available), or an API has changed and won't work as expected. I'm not actually sure how this would work, and am interested in a deeper case study, but I don't think that's necessary to get to a conclusion here.

Now, I can see how just dropping bluebird might seem to sidestep the issue, but then it's just another subtle problem, because we wrap sequelize and our code base calls into sequelize, we wouldn't be exposing the actual real type of the promises that sequelize returns, and if we ever create a promise to do work, it could (would it have to be?) be native instead of bluebird, which would mix promise types. Again, not 100% on all that, but just thinking about it makes me squirm.

@schmod if I've mistakenly analyzed anything, please rebuketh me! #learning #discussion

=> keep bluebird, match its version to sequelize

RobinBuschmann · 2017-11-14T19:48:23Z

Ok, lets define an explicit dependency of @types/bluebird. The question is, do we need to set it as a peer dependency or not? To ensure that the @types/bluebird version matches the version required by sequelize, we need to use the exact same version, right? So if this is needed anyway, is there a benefit from using it as peer dependency?

snewell92 · 2017-11-15T14:34:53Z

Good question. If we list bluebird as a peer dep, we force the user to use bluebird. Since sequelize doesn't do that, I don't think we should do that. We should report correct types from sequelize, since they use bluebird, but I think it would be fine if users use native promises, then they would just lose out on bluebird features but still be able to use Promise.all, p.then, Promise.resolve and friends, as I think bluebird was designed with that in mind (progressive library, legacy user).

Just an example package.json so I can wrap my head around it more:

package.json snippet

  "dependencies": {
    "@types/sequelize": "^4.0.76",
    "bluebird": "^3.4.6",
    "@types/bluebird": "^3.5.5"
  },
  "peerDependencies": {
    "sequelize": "4.22.6"
  },

@schmod @RobinBuschmann perhaps our sequelize peer dep should just be a minimum range? like 4.22.x? Idk how strict we want to be. Pinning sequelize to a specific version makes it easier for us, but less flexible for the end user. I like having @types be flexible so we don't have to bump up anytime someone lands a PR into sequelize typings, but sequelize might be a little hairier... perhaps we can rely on sequelize following semver, and just take patches?

schmod · 2017-11-15T17:18:28Z

I honestly don't see a problem being somewhat permissive with the version ranges of our sequelize dep (whether it's a peer dep or regular dep). I think something like "~4.22.0" would be totally acceptable.

@types/sequelize, on the other hand, has historically caused us a lot of problems, because their "minor" releases have sometimes made major updates to the typings, which is obviously risky for us, given our extremely close and complicated relationship with those types. I think we need to specify an exact version of @types/sequelize and bluebird.

snewell92 · 2017-11-20T14:40:54Z

Ah, okay. So something like this:

package.json snippet:

  "dependencies": {
    "bluebird": "3.4.6",
    "@types/bluebird": "3.5.15",
    "@types/sequelize": "4.0.76"
  },
  "peerDependencies": {
    "sequelize": "~4.22.0"
  }

What do y'all think? 👍 for ya, 😕 for wat

snewell92

Change package.json as per our consensus. Then LGTM 😀

RobinBuschmann · 2017-12-09T09:25:37Z

Just this week I've noticed this tsc feature: https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#types-typeroots-and-types

So my question: Should we use this and put the type dependencies (@types/bluebird, @types/sequelize) in our build to minimize dependencies of this package. What do you think? Any experience with this? @schmod @snewell92

snewell92 · 2017-12-18T22:17:31Z

I sort have used typeRoots, but not in a library context.

I'd be interested to see how typescript handles several cases, I might have time to play with this kind of stuff next week. I would try by specifying the type roots, and then specifying no type roots, and having a consuming project both match and then not match the bluebird / sequelize typings - just to see what happens.

I don't think the use of typeroots will solve the problem though.

RobinBuschmann · 2017-12-19T19:54:42Z

I'd be interested to see how typescript handles several cases, I might have time to play with this kind of stuff next week. I would try by specifying the type roots, and then specifying no type roots, and having a consuming project both match and then not match the bluebird / sequelize typings - just to see what happens.

This would be nice :)

I don't think the use of typeroots will solve the problem though.

Wasn't intended to be a fix for this issue. Probably not the right place to mention this feature.

RobinBuschmann · 2019-02-21T13:29:15Z

With version 1.0.0-alpha.8 sequelize-typescript uses the typings bundled with sequelize. So the @types/sequelize and @types/bluebird dependencies are removed there.

Andrew Schmadel added 2 commits October 13, 2017 09:11

permissive versioning of 3rd-party types

31b612b

make tests pass with new typings

ba1880e

schmod commented Oct 13, 2017

View reviewed changes

snewell92 reviewed Oct 13, 2017

View reviewed changes

RobinBuschmann reviewed Oct 14, 2017

View reviewed changes

RobinBuschmann mentioned this pull request Oct 14, 2017

Options is missing operatorsAliases #167

Closed

RobinBuschmann mentioned this pull request Oct 16, 2017

Sequelize class missing Op #169

Closed

schmod mentioned this pull request Nov 3, 2017

Type inference for static Model methods #189

Merged

snewell92 suggested changes Dec 8, 2017

View reviewed changes

RobinBuschmann closed this Feb 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loosen semver ranges on 3rd-party typings #166

Loosen semver ranges on 3rd-party typings #166

schmod commented Oct 13, 2017

schmod Oct 13, 2017

schmod Oct 13, 2017

snewell92 Oct 13, 2017 •

edited

Loading

schmod Oct 13, 2017

codecov-io commented Oct 13, 2017 •

edited

Loading

snewell92 Oct 13, 2017

RobinBuschmann Oct 14, 2017

snewell92 commented Oct 13, 2017

RobinBuschmann Oct 14, 2017

RobinBuschmann commented Oct 14, 2017

snewell92 commented Oct 16, 2017

schmod commented Oct 16, 2017 •

edited

Loading

snewell92 commented Oct 16, 2017

RobinBuschmann commented Oct 16, 2017

schmod commented Oct 19, 2017

RobinBuschmann commented Oct 20, 2017 •

edited

Loading

snewell92 commented Nov 14, 2017

RobinBuschmann commented Nov 14, 2017 •

edited

Loading

snewell92 commented Nov 15, 2017

schmod commented Nov 15, 2017 •

edited

Loading

snewell92 commented Nov 20, 2017

snewell92 left a comment

RobinBuschmann commented Dec 9, 2017 •

edited

Loading

snewell92 commented Dec 18, 2017

RobinBuschmann commented Dec 19, 2017

RobinBuschmann commented Feb 21, 2019


		return arg.reduce((models: any[], dir) => {

		if (typeof dir !== 'string') {

Loosen semver ranges on 3rd-party typings #166

Loosen semver ranges on 3rd-party typings #166

Conversation

schmod commented Oct 13, 2017

schmod Oct 13, 2017

Choose a reason for hiding this comment

schmod Oct 13, 2017

Choose a reason for hiding this comment

snewell92 Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

schmod Oct 13, 2017

Choose a reason for hiding this comment

codecov-io commented Oct 13, 2017 • edited Loading

Codecov Report

snewell92 Oct 13, 2017

Choose a reason for hiding this comment

RobinBuschmann Oct 14, 2017

Choose a reason for hiding this comment

snewell92 commented Oct 13, 2017

RobinBuschmann Oct 14, 2017

Choose a reason for hiding this comment

RobinBuschmann commented Oct 14, 2017

snewell92 commented Oct 16, 2017

schmod commented Oct 16, 2017 • edited Loading

snewell92 commented Oct 16, 2017

RobinBuschmann commented Oct 16, 2017

schmod commented Oct 19, 2017

RobinBuschmann commented Oct 20, 2017 • edited Loading

snewell92 commented Nov 14, 2017

RobinBuschmann commented Nov 14, 2017 • edited Loading

snewell92 commented Nov 15, 2017

schmod commented Nov 15, 2017 • edited Loading

snewell92 commented Nov 20, 2017

snewell92 left a comment

Choose a reason for hiding this comment

RobinBuschmann commented Dec 9, 2017 • edited Loading

snewell92 commented Dec 18, 2017

RobinBuschmann commented Dec 19, 2017

RobinBuschmann commented Feb 21, 2019

snewell92 Oct 13, 2017 •

edited

Loading

codecov-io commented Oct 13, 2017 •

edited

Loading

schmod commented Oct 16, 2017 •

edited

Loading

RobinBuschmann commented Oct 20, 2017 •

edited

Loading

RobinBuschmann commented Nov 14, 2017 •

edited

Loading

schmod commented Nov 15, 2017 •

edited

Loading

RobinBuschmann commented Dec 9, 2017 •

edited

Loading