I usually find it help to start with the API we want, and work backwards. For models, we want something like:
import Post from '../models/post';
class CreatePost extends Action {
respond({ body }) {
let post = new Post(); // <- Just a simple `new Post()`
return post.save();
}
}
// or ...
import Post from '../models/post';
class ListEditablePosts extends Action {
respond() {
return Post.editableBy(this.currentUser); // <- Static query methods
}
}
Right now, that's not possible with Denali. The main reason is because of our test framework, Ava, and how we do dependency injection.
Ava runs tests concurrently. This means that tests cannot rely on any shared state, because they execute at the same time. If they do share state, you get very subtle and bizarre test failues, because test B modifies things that test A assumes don't change. A concrete example:
// container is shared across tests:
test('A', async (t) => {
container.mock('orm-adapter', {
findAll() { return [ 1, 2, 3 ]; }
});
let results = await Post.all();
t.equal(results.length, 3);
});
test('B', async (t) => {
container.mock('orm-adapter', {
findAll() { return []; }
});
let results = await Post.all();
t.equal(results.length, 0);
});
In this example, assume Post.all() method needs to talk to the 'orm-adapter'. It gets a
reference to that orm adapter via dependency injection through the shared container.
Notice that the two tests provide different mocks for the orm adapter, to test different
behaviors. However, both tests use the same Post class.
Remeber that Ava runs tests concurrently. So when test A runs, it sets up it's mock
of orm-adapter, then invokes Post.all(). But meanwhile, test B has started, and it
mocks the orm-adapter as well - overwriting test B mocks.
The result? Test B fails saying results.length is zero - because it used the mock from test A!
The fix? We need a separate container for each test (easy enough), and we can't allow the static,
shared Post class to see it, because there's only one Post class and it therefore
can't be used concurrently, because it wouldn't know which container to use.
This is why, right now, you can't simply import Post and use it - because you'd be in
danger of leaking state through the shared use of the static class. So we currently hide
all the static classes behind the this.db service, which handles them appropriately.
Now we don't get the APIs we want, as mentioned above (i.e. Post.all(), Post.editableBy(), new Post() etc).
So how do we fix this while avoiding the issues described above?
Let's start from scratch, putting aside the pitfalls from above. All those APIs we want depend on
being able to use the static Post class - so we know we at least need to get a reference to Post somehow.
So where can it come from?
In JS classes like Denali uses, we can get references to variables from 4 places really:
We could do something like this.models.Post, but that means lots of repetitive code like
let { Post } = this.models in every single action - kinda sucks. That's framework code
for the sake of the framework rather than your app logic. This doesn't seem much better than bolting on
to the existing this.db API.
This feels like a non starter - all the boilerplate of instance methods, plus now we're polluting method signatures with seemingly unrelated arguments as well.
Globals run counter to the general Node ethos, and tend to lead to "where the heck did this Post variable come from", making the application harder to reason about and debug. Also, globals mean shared state - which as we saw above is problematic.
This avoids the drawbacks from above, and fits in well with existing tooling. But it suffers from the pitfalls we encountered above.
One solution might be to ditch support for concurrent application instances in memory (i.e. ditch Ava, or at least force it serial). That way, the shared state is okay - because there will only ever be one container in memory, one set of mocks, etc. I call this the "single-tenant" solution because it means only one Denali app can exist in an Node process at a time.
The big drawback here is that we give up a really awesome feature for large apps - automatically concurrent test suites. Can we do better?
Honestly, we can't really do better than the single-tenant solution if we stay inside the constraints of JS. But - we have a build step with Denali. We can go beyond JS.
I call this solution the "closure-global" solution, because we basically create a pseudo global scope using closures added at build time.
We would use a bundler (potentially something like Rollup), which would combine the entire app & all it's dependencies into a single bundle.js file. Then, we simply wrap the entire file in a function definition. This turns the entire source code into a factory.
Our problems with import Post stem from the fact that everyone gets the exact same Post,
which means they share state - our big no-no with Ava. If we want to keep Ava's concurrent tests,
we need a different Post per application instance.
One approach might be to hijack Node's require (because it's the source of our shared state -
it caches modules and returns the cached result). We could re-evaluate the Post module
every time it's required.
But this won't work for us, because within an application instance,
we do want the Post model to be shared. And since require() has no knowledge of which application
instance we care about, it's impossible for it to know whether to deliver a shared copy or a fresh copy.
By turning the entire application source into a factory, we are effectively bypassing Node's module caching when
loading the app from outside it's own source code (i.e. from a test, or the index.js entry point). But inside
the application source code, we would effectively duplicate Node's caching behavior with our bundler.
We can create fresh copies of all the class definitions anytime we need it (i.e. per test) by simply
running the factory function to generate fresh definitions of the entire application. This lets us freely share
state across static class definitions within the application, but our tests should work fine, because they each
will have a different copy of Post to work with.
Basically, we're "stepping down" the global scope via a "pseudo-global" scope (the factory function closure's scope)
So when the framework kicks off, we'd eagerly load everything through the container, injecting
the static class definitions with a container reference that is unique to that closure, which
gets us our Post.all(), new Post(), etc., with no worries about sharing state.
One side effect of this bundle step - we could ditch main-dir and simply intercept/rewrite the
lookup paths as we bundle. This would be nice since we can avoid the annoying (albeit rare) footgun
that running Denali requires importing main-dir first.
By moving the container to this pseudo global scope, we could move container injections out of the class definitions and into the module definitions:
class CreatePost extends Action {
mailer = inject('service:mailer');
respond() {
this.mailer.send();
}
}
Could become:
const mailer = lookup('service:mailer');
class CreatePost extends Action {
respond() {
mailer.send();
}
}
This is probably better for performance (if we can avoid injecting every new instance and instead rely on the module closure scope, we don't need to scan every new instance for injections). It's a one time startup cost as each module is evaluated vs. per instance runtime cost.
If we wanted to, we could take this one step further and open up a lot of cool API
improvements too, like getting rid of the inject/lookup helper entirely. We could do this by
hijacking require() in the closure scope and have it check the container first:
import mailer from '../services/mailer'
turns into:
const mailer = require('../services/mailer').default;
But the require() there would actually be Denali's require:
function require(path) {
if (shouldResolveFromContainer(path)) {
container.lookup(specifierFromPath(path));
} else {
originalRequire(path);
}
}
This would let us do stuff like:
import mailer from '../services/mailer';
class CreatePost extends Action {
respond() {
mailer.send(...);
}
}
But we would still retain all the DI goodness for testing:
test = unitTest('action:posts/create', {
'services:mailer': mailerMock
});
test('sends mail', async (t) => {
let action = t.subject;
action.respond();
t.equals(mailerMock.sentEmails.length, 1);
});
The downsides being that (a) this would probably break types, since TS would think
that import '../services/mailer' is the static class, but it would actually be
the singleton, and (b) more magic (hijacking require is unexpected and surprising).
Your built code would look different than your source code:
- one big file for all your Denali / containered code
- the factory wrapper + some container managment would suddenly appear in compiled output
This could be a big change, depending on how difficult the hypothetical bundler is to implement.
@knownasilya pointed out that this might spike memory usage beyond CI limits during tests