refactor(ember-5): access Ember Models via Store

[Pixelik]

Refactor

• ember-data 5.x only allows accessing Models via the Store
• This refactoring removes the following Deprecation Warning:

Instead of accessing a Model with:

// ember-cli-mirage/ember-data.js

let modelName = matches[1];
let model = require(path, null, null, true).default

It will now be accessed with:

// ember-cli-mirage/ember-data.js

let modelName = matches[1];
let model = application.__container__.lookup('service:store').modelFor(modelName)

[SergeAstapov]

@Pixelik sorry for delay here, do you mind to refresh branch with latest master to get CI going and be green?

[Pixelik]

Hello. It all looks green to me and up-to-date, ready to be merged 🤔

[SergeAstapov]

Hello. It all looks green to me and up-to-date, ready to be merged 🤔

branch of this PR is outdated and although the PR status is green, it has no job runs in "Checks" tab.

Anyways, I've created a unit test which uses discoverEmberDataModels function in #641 and yes it fails for embe-data@5 scenarios.
However, the fix suggested here does not work (note that in #2528 I've cherry-picked changes proposed in this PR), because application.__container__ is undefined.

Hence, for discoverEmberDataModels function compatibility with Ember Data v5 we need a different solution.

[SergeAstapov]

this actually does not work, see CI run https://github.com/miragejs/ember-cli-mirage/actions/runs/6193523102

[Pixelik]

@SergeAstapov where is this test defined ? I can't find it in this branch 🤔

Do you have a link to that file ?

(But it makes sense that such a test would fail as this PR is changing the way tests are discovered)

[SergeAstapov]

@Pixelik there is no such test in master now and this PR does not add anything.

@Pixelik please see https://github.com/miragejs/ember-cli-mirage/pull/2528/files#diff-a16b283fd8b39b8decc1f1e8824d3a777dca6c861c511f6999ace605541c8807 where test added

[Pixelik]

@SergeAstapov
Oh I see you created a separate PR with tests about this
Your unit test is not going to work because the required initializers won't run during your unit test.
👉 You can only test this by first having an App instance running in your test.

[SergeAstapov]

@Pixelik well, yes. but how can we guarantee that it will be present by the time discoverEmberDataModels() is invoked in mirage config?

[Pixelik]

If I understand ember initializers correctly, it is impossible that app/mirage/config.js will be executed before app/initializers/ember-cli-mirage.js is executed

[Pixelik]

I have already tried my solution in my app and it works by the way, but obviously this, by itself, is not good enough of a proof that it will always work, I'm just mentioning this as a side note.

[SergeAstapov]

@Pixelik sounds like this limitation needs to be documented then?

[Pixelik]

Perhaps. Is it possible that someone would try to discoverEmberDataModels() not from within an Ember app (or tests/dummy/app) that has already been initialized ? 🤔 Sounds weird to me but.. 🤷‍♂️

EDIT: Well I guess only if they invoke this method during their Unit tests..

[Pixelik]

But yes, documenting this sounds necessary to me now as well.

[cah-brian-gantzler]

Sorry for being late to the party but I would think that autoDiscoverDataModels should be passed the store.

In setupMirage the this.owner is passed into startMirage (so if startMirage is called outside tests, this is a required param). https://github.com/miragejs/ember-cli-mirage/blob/master/packages/ember-cli-mirage/addon-test-support/setup-mirage.js#L23, so it will be accessible

(Quick aside, makeServer can be passed as an option on SetupMirage and passed into StartMirage)

import makeServer from 'app/mirage/config';

setupMirage(hooks, { makeServer });

then in startMirage, if one is passed, it uses it, if not looks it up. https://github.com/miragejs/ember-cli-mirage/blob/master/packages/ember-cli-mirage/addon/start-mirage.js#L21 (This look up is what was prepped in one of the initializers. I would love to see a mandate of passing it in, then could remove one of the intializers). This also has the benefit of specifying different configs for different tests if needed, or a different config for dev vs test. hence why testConfig is no longer needed.

You could lookup the store and add it here https://github.com/miragejs/ember-cli-mirage/blob/master/packages/ember-cli-mirage/addon/start-mirage.js#L29 or add owner here and require them to look it. Either way you now have a store that you could pass into autoDiscoverDataModels which might be more straight forward and intuitive. Supports multiple stores if someone did have them, plus you dont depend on the initializer for application.__container__. (do you even need the initializer?)

[SergeAstapov]

Thank you @cah-brian-gantzler and @Pixelik for ideas and discussion! I've landed fix in #2542