Move HTTP strategies and DB adapters into separate packages

[carlbennettnz]

Having express and mongoose support in the main package means we need to have them listed as peerDependencies, which causes warnings if you're using the library with a different strategy/adapter.

[ethanresnick]

Not sure I follow... Shouldn't the checks in https://github.com/ethanresnick/json-api/blob/master/src/index.ts mean that you don't need to list/install the peer dependencies for any adapters or strategies you're not using?

[carlbennettnz]

They stop runtime errors, but npm still complains about the missing peer dependencies on install

[ethanresnick]

Gotcha. Hmm... the warnings aren't great, but do they stop the install or does npm continue successfully? Assuming the latter, pulling out these packages to remove the warnings would be nice, but it's not critical and it comes at the cost of complicating the install/setup process for people who do want to use one of the built-in adapters, so idk.

[carlbennettnz]

I see what you're saying. I guess I'm thinking of knex as an example. Knex works with a bunch of SQL databases, but the adapters aren't bundled. To have it work with Postgres, for example, you need npm install knex pg.

But I agree it's not a biggy. And you're right that it doesn't stop the installation.

[ethanresnick]

After thinking about this, I'm 👍 on the idea. I'm gonna move the Mongoose Adapter out as part of v3. I've also registered the @json-api scope on npm, so ideally it could hold all the adapters/related packages. Would you want to publish your Postgres adapter there? If so, I'm happy to give you publish rights on npm.

[carlbennettnz]

Sweet. There are still a few things that are missing from my adapter (polymorphic types, add/remove from relationship, probably a few other smaller things) that I should add before other people use it. I also want to tidy up the structure used for schema definitions. That said, happy to release a beta.

[carlbennettnz]

Want me to set up a repo for @json-api/mongoose-adapter and make a PR to remove it from here? I cry a little inside every time I install mongoose in an app using the knex adapter just to make tsc happy.

[ethanresnick]

@carlbennettnz Sure! If you make the PR, I'm totally game. I've created a repo for the mongoose adapter and added you as a collaborator. Please PR any changes to this repo, but then feel free to commit the initial code directly to the new mongoose adapter repo.

Thanks for this!

[carlbennettnz]

Sweet, I'll get started!

The adapter's tests currently check behaviour only after passing requests and responses through the rest of the json-api stack. This should change, right? We should be building Query objects in tests and passing them directly to the adapter.

As for this repo, its tests currently depend on the Mongoose adapter. I'm more on the fence about how to handle this. We could just include the Mongoose adapter as a dev dependency and continue as we are. Alternatively we could build a simple mock adapter with an in-memory database or we could manually tell the adapter what its response should be test-by-test.

Any preferences?

[ethanresnick]

As for this repo, its tests currently depend on the Mongoose adapter. I'm more on the fence about how to handle this. We could just include the Mongoose adapter as a dev dependency and continue as we are. Alternatively we could build a simple mock adapter with an in-memory database or we could manually tell the adapter what its response should be test-by-test.

My gut is that a stub adapter with responses configured test-by-test is probably the best option, in that it gives us test isolation and would let us run the tests in parallel. The only obstacle to that, I think, is that some of the tests are currently written with the idea that they will run in order, with a real persistence layer; I'm looking at (e.g.) these guys. So, I'd say: try to use a stub but, if it's easier at any point to write a dummy adapter with in memory db than to refactor the tests, feel free to do that.

The adapter's tests currently currently check behaviour only after passing requests and responses through the rest of the json-api stack. This should change, right? We should be building Query objects in tests and passing them directly to the adapter.

Yeah. All these tests should probably be split in two: one in the main package that uses the stub/in-memory adapter, as a way to test implicitly that the json-api stack constructs the Query properly, and then one as a pure unit test in the adapter package where we feed in the Query directly and assert on the result.

[carlbennettnz]

Sounds good 👍

[carlbennettnz]

I've started transforming the tests to work with the adapter directly, rather than through the full stack. If you've got a minute, I wouldn't mind getting some quick feedback on this testing style. Don't want to go too far down the wrong track.

https://github.com/ethanresnick/json-api-mongoose-adapter/tree/master/test/integration/add-to-relationship

[ethanresnick]

@carlbennettnz Just took a look at the file you linked. It looks great to me!

Thanks a lot for your help here :)

[carlbennettnz]

Tests are all done now on the MongooseAdapter side. The last thing I can think of that needs doing over there is sorting out all of the the imports of json-api internals.

Here's what I'm thinking we should do with each of them. Does this look about right to you?

// Export from json-api
import {
  Adapter,
  TypeIdMapOf,
  TypeInfo,
  FindReturning,
  CreationReturning,
  UpdateReturning,
  DeletionReturning,
  RelationshipUpdateReturning,
  ReturnedResource
} from "json-api/build/src/db-adapters/AdapterInterface";
import { ResourceWithTypePath } from "json-api/build/src/types/Resource";
import { SupportedOperators, Identifier } from "json-api/build/src/types";
import RelationshipTypeDocumentation from "json-api/build/src/types/Documentation/RelationshipType";

// Only used in the MongooseAdapter anyway, so just move them
import { setDifference, reduceToObject } from "json-api/build/src/util/misc";
import { getTypeName } from "json-api/build/src/util/naming-conventions";

// Use lodash: _.value(), _.groupBy(), _.unset()
import { values as objectValues } from "json-api/build/src/util/objectValueEntries";
import { partition, deleteNested } from "json-api/build/src/util/misc";

// Split up, so adapter-related parts are moved into the mongoose-adapter package
import * as Errors from "json-api/build/src/util/errors";

// Just a utility type, duplicate
import { StrictDictMap } from "json-api/build/src/types";

// This one is already exported by json-api, but if I try to use it instead of the this import,
// TypeScript simultaneously complains that I have an unsed import and that the type
// FieldExpression could not be found. I have no idea why that would happen.
import { FieldExpression } from "json-api/build/src/types";

[ethanresnick]

All of your comments look right, with the exception that some of the types around query parameter parsing (i.e. FieldExpression and Identifier), should probably be imported from here into the MongooseAdapter and into the main json-api package (which could re-export some of them as needed for back-compat). Also, I think the FieldExpression type currently in json-api package should probably go away, as it doesn't reflect the runtime reality if the user has customized the query parsers.

Thoughts?