Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uncaught Error: You must pass owner to startMirage() #2212

Open
timiyay opened this issue Jul 17, 2021 · 21 comments
Open

Uncaught Error: You must pass owner to startMirage() #2212

timiyay opened this issue Jul 17, 2021 · 21 comments

Comments

@timiyay
Copy link

timiyay commented Jul 17, 2021

When converting an Ember 3.24 app to use embroider, we can no longer boot the app from the Ember dev server.

The app crashes in the browser, with an error:

Uncaught Error: You must pass `owner` to startMirage()
    at startMirage (start-mirage.js:34)
    at startMirage (ember-cli-mirage.js:59)
    at Object.initialize (ember-cli-mirage.js:53)
    at index.js:124
    at Vertices.each (dag-map.js:231)
    at Vertices.walk (dag-map.js:145)
    at DAG.each (dag-map.js:75)
    at DAG.topsort (dag-map.js:83)
    at App._runInitializer (index.js:151)
    at App.runInitializers (index.js:122)

I will endeavour to create a repo to reproduce the issue - the contributing guidelines pointed me to https://github.com/miragejs-archive/ember-cli-mirage-boilerplate which appears unmaintained, and is using classic Ember code (our issue is on an Octane 3.24 app).

@timiyay timiyay changed the title Uncaught Error: You must pass owner to startMirage() Uncaught Error: You must pass owner to startMirage() Jul 17, 2021
@timiyay
Copy link
Author

timiyay commented Jul 17, 2021

Digging through the stack trace, it looks like the issue arises from:

export default function startMirage(owner, { env, baseConfig, testConfig, makeServer } = {}) {
if (!env || !baseConfig) {
if (!owner) {
throw new Error('You must pass `owner` to startMirage()');
}

Our app doesn't currently export default from mirage/config.js, which results in baseConfig being undefined, which causes the error.

This issue isn't encountered when using the classic Ember builds, it triggers once were using embroider and webpack.

Is it a requirement that Mirage consumers export a default function?

Currently, we export a named makeServer function.

@timiyay
Copy link
Author

timiyay commented Jul 17, 2021

Looks like this has come up briefly in Ember Discord in April:

Problem: https://discord.com/channels/480462759797063690/568935504288940056/834541111065313330
OP Solution: https://discord.com/channels/480462759797063690/568935504288940056/834876984416141322

Ed Faulkner's explanation: https://discord.com/channels/480462759797063690/568935504288940056/834888941881655306

ef4: I've seen this before. I don't know if it's only a docs problem or an implementation problem, but mirage trolls people with the distinction between export default function and export function makeServer, and I think whatever they're doing works by accident in traditional builds but violates the ES module spec.

@cah-brian-gantzler
Copy link
Collaborator

The original version of mirage does require an export default from the config. Thats where it looks for routes.

When Mirage was abstracted away to miragejs, I implemented a makeServer function that if you define, will no longer use the export default config, but instead use the makeServer to define the server which is very similar to the way miragejs now defines the server. The default export is there for backward compatibility, but hopefully people would start using the makerServer version.

Since Sam is considering doing away with this addon, I am working on a replacement that would still provide emberish functionality, and part of that is removing the legacy export default being just the routes and instead be the makeServer function.

If that is the only issue you are currently having with embroider I think we are ok.

As a curiosity, what does your current config.js look like?

@timiyay
Copy link
Author

timiyay commented Jul 19, 2021

Thanks for taking a look!

A redacted version of our config is:

import appConfig from 'my-app/config/environment'
import { createServer, discoverEmberDataModels } from 'ember-cli-mirage'

/**
 * Pass all route handlers here, to be used in `makeServer` fn.
 */
function routes() {
  this.passthrough('/write-coverage')

  this.post('/my-api/resource')
  this.delete('/my-api/resource/:id')
  this.get('/my-api/resource/:id')
  // and so on, for all Mirage routes used in development
}

/**
 *  Defines a centralized Mirage Server
 *
 * @param {Object} config - contains all modules that exist in the project's /mirage directory
 * @returns {Object} - The mirage server
 */
export function makeServer(config) {
  // `config` does not contain autogenerated Mirage model definitions by default
  // `discoverEmberDataModels` will add the models to config, so that Mirage schema is automatically inferred from the Ember Data models and relationships
  const models = discoverEmberDataModels(config)
  const namespace = appConfig.APP.apiNamespace

  const finalConfig = { ...config, models, routes, namespace }

  return createServer(finalConfig)
}

I've included comments from other devs, which may've informed my conception that Mirage is auto-generating models that could present problems in Embroider's static world.

Note that the above config will trigger the reported error. To avoid this I have to add the following:

export default function () {
  // no-op function to appease Mirage
}

So it's possible this is a config bug? It seems like Mirage is still trying to configure an export default function, even if makeServer is present, and exploding when export default isn't there?

@timiyay
Copy link
Author

timiyay commented Jul 20, 2021

Adding the empty export default fixes this immediate issue.

A side issue is that it causes a string of test failures when building via embroider, with the error message:

"You created a second Pretender instance while there was already one running. Running two Pretender servers at once will lead to unexpected results and will be removed entirely in a future major version.Please call .shutdown() on your instances when you no longer need them to respond.", source: http://localhost:7357/testem.js (967)

I'm not yet sure if this is caused by the empty function, or whether there's something else Mirage is doing by default in tests that's causing it.

@cah-brian-gantzler
Copy link
Collaborator

discoverEmberDataModels does not take a parameter, it just discovers all the ember data models. If you have models you have also defined in mirage you will need to do the following

Your make server should be

export function makeServer(config) {
  // `config` does not contain autogenerated Mirage model definitions by default
  // `discoverEmberDataModels` will add the models to config, so that Mirage schema is automatically inferred from the Ember Data models and relationships
  const models = { ...discoverEmberDataModels(), ...config.models }
  const namespace = appConfig.APP.apiNamespace

  const finalConfig = { ...config, models, routes, namespace }

  return createServer(finalConfig)
}

If you have the same model name in ember data and in mirage, the mirage one will win since config.models is last.

The second Pretender instance is usually caused by mirage being started twice. If this is caused by embroider and you find the cause please let me know. Right now I get it when I do the tests wrong, like do a setupMirage in a parent and and a nested module for example.

Lastly I guess I wasnt smart enough by doing some conditional require for the default, I plan for the makeServer to be the default and the only way to config. Right now though they are both there for backward compatibility. So the docs might say you dont have to export a default (but I guess under embroider you do until we deprecate and remove it). What this addon currently does is look for a default export and a makeServer function. If there is a makeServer function if will use that to create the server, if not it will use its internal logic to create a server. So you can make the routes function an export default, you dont have to create a second empty one.

Once Mirage was extracted to MirageJS, many of the exports are just re-exports (https://github.com/miragejs/ember-cli-mirage/blob/master/addon/index.js#L2) and we plan to drop them and have comsumers directly import them from miragejs.. I cant figure out yet how to deprecate an export. Until then, I would highly suggest not importing from ember-cli-mirage that could be imported from mirageJs directly. So for example, a model defined in my mirage looks like this. You should also add miragejs to your package.json since you will not be referring to it directly.

import { Model, belongsTo } from 'miragejs';

export default Model.extend({
  role: belongsTo(),
  user: belongsTo(),
});

Note that you have to use the extend syntax. It doesnt seem to work with class syntax and thats a mirageJS concern. You can import factories, traits etc.

The main things that are provided by ember-cli-mirage is the setupMirage, startMirage, and the discoverEmberDataModels function.

@timiyay
Copy link
Author

timiyay commented Jul 21, 2021

Thankyou so much for all your suggestions @cah-briangantzler.

I've made a proposed fix PR in #2213 for the specific issue reported here.

I'm still getting myriad test failures related to miragejs, , even after I amend my Mirage config, and consume the fix in the PR. I'm unable to understand them well enough to file new issues just yet. They may end up being user error.

The issues still include myriad test failures relating to 2nd instances of Pretender being started, and I can confirm that we're not starting Mirage multiple times.

In all cases we call

import { setupMirage } from 'ember-cli-mirage/test-support';

module('my ember test', function (hooks) {
  setupMirage(hooks)

I'm also getting regular, but flaky, test failures with the message

Uncaught (in promise) Error: Could not find module `miragejs` imported from `(require)`
    at missingModule (vendor.js:254)
    at findModule (vendor.js:265)
    at requireModule (vendor.js:31)
    at eval (webpack://my-ember-app/../externals/miragejs.js?:1)
    at Object.../externals/miragejs.js (chunk.01bb864983865af28a5d.js:906)
    at __webpack_require__ (chunk.01bb864983865af28a5d.js:940)
    at eval (webpack://my-ember-app/./tests/my-test.js?:10)
    at Module../tests/my-test.js (chunk.01bb864983865af28a5d.js:447)
    at __webpack_require__ (chunk.01bb864983865af28a5d.js:940)
    at Module.eval [as callback] (webpack://my-ember-app/./assets/test.js?:118)

...which are triggered from the following import statement inside a test file:

import { Response } from 'miragejs'

Again, I think these additional details are out of scope for the reported issue, and I'll open issues if I can get shareable repros happening.

@rahulk94
Copy link
Contributor

rahulk94 commented Dec 20, 2021

Hey @timiyay and @cah-briangantzler did either of you find a resolution for

"You created a second Pretender instance while there was already one running. Running two Pretender servers at once will lead to unexpected results and will be removed entirely in a future major version.Please call .shutdown() on your instances when you no longer need them to respond.", source: http://localhost:7357/testem.js (967)

We're getting the same thing once we turn on staticAddonTestSupportTrees in our Embroider configuration and its not obvious to me if we're doing anything wrong 🤔 . Fwiw let me know if ya'll reckon there should be a separate issue raised for this.

@cah-brian-gantzler
Copy link
Collaborator

I have not seen the two server pretender problem. I would think that would come from calling setupMirage twice in the same test, or the setupMirage failing to hook into the afterEach to shut down at the end of the test.

Looking back over this I see we were talking about the default export. When I created the makeServer and the way it was exposed I was unaware this was going to cause an issue for embroider. That has since been fixed in that if the default export takes 2 params, it assumes that it is the make server function. However I have been unable to actually release a new version of ember-cli-mirage containing this.

@rahulk94
Copy link
Contributor

Yeah I'm not entirely sure what we've got wrong tbh. I've compared with https://github.com/xg-wang/ember-realworld-fastboot which also uses Mirage but that seems to work perfectly fine. We're not using makeServer, just exporting the default function with routes in it.

I'll keep digging and see if theres anything funky we're doing.

@bistin
Copy link

bistin commented Jan 3, 2022

Hi @rahulk94 , I have the same "You created a second Pretender instance" problem on an embroider project. After tracing code, I find the setting

'ember-cli-mirage': {
    enabled: false,
},

can turn off the warning. And I find https://github.com/xg-wang/ember-realworld-fastboot/blob/master/config/environment.js#L41 have the same setting;

@cah-brian-gantzler
Copy link
Collaborator

That setting is saying to disable ember-cli-mirage completely. By default it is on in tests and on in dev (if --proxy is not passed) and off in production. see https://www.ember-cli-mirage.com/docs/advanced/environment-options#enabled

By setting the enable to false you are disabling ember-cli-mirage all the time. Not a fast boot user but somehow two were being created and by setting this you are stopping the one. What Im now curious of is, how is the second one getting created since the setting is telling ember-cli-mirage to be disabled all the time :)

@rahulk94
Copy link
Contributor

rahulk94 commented Jan 10, 2022

Woah @bistin that is so weird... Thanks so much for the tip. I've disabled this in our app along with a comment referring readers here.

@cah-briangantzler yup so weird. It def seems like some quirk of using Mirage and Fastboot together. Fwiw we're currently using [email protected]. I'd love to dig into this further but don't have time atm.

@cah-brian-gantzler
Copy link
Collaborator

Are you able to close this issue?

@mrloop
Copy link

mrloop commented Oct 3, 2022

Came across this issue while updating ember data from 3.28 to 4.7.1. In the test suite I am seeing

"You created a second Pretender instance while there was already one running. Running two Pretender servers at once will lead to unexpected results and will be removed entirely in a future major version.Please call .shutdown() on your instances when you no longer need them to respond."

Debugging the app and only changing the ember data version I get to

which is starting the extra instance in 4.7.1 but not 3.28.

The instance that I want to start is in a customisable test setup http://emberjs.github.io/rfcs/0637-customizable-test-setups.html e.g.

export function setupApplicationTest(hooks, options) {
  setupMirage(hooks);
}

Looking further up the stack I get to

if (dependencySatisfies('@ember/test-helpers', '*') && isTesting()) {
isTesting is returning false in 4.7.1 but true in 3.28 not sure how to debug any further

@mrloop
Copy link

mrloop commented Oct 3, 2022

Using the suggested workaround works for me

'ember-cli-mirage': {
    enabled: false,
},

@cah-brian-gantzler
Copy link
Collaborator

Where did you get the suggested workaround?

@mrloop
Copy link

mrloop commented Oct 3, 2022

Earlier in this thread #2212 (comment)

@cah-brian-gantzler
Copy link
Collaborator

cah-brian-gantzler commented Oct 3, 2022

Thats embarrassing :)

That flag is one I hope to remove one day and not have ember-cli-mirage make assumptions about what should happen. By having you specifically call code when you want mirage to run in production or development it will be easier to tell what is happening and why. It will also be easier to specify different routes and data for each environment

@mrloop
Copy link

mrloop commented Oct 5, 2022

Have tracked my issue with two pretender instances starting due to isTesting returning false to unrelated issue with different addon emberjs/ember-classic-decorator#99

@cah-brian-gantzler
Copy link
Collaborator

Nice. I have seen this issue reported before. Thanks for all the effort, hope it gets resolved there. Thanks for the link, I will follow the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants