Move mirage folder outside of addon folder

[bertdeblock]

By default everything included in the addon folder ships. Since Mirage is a mocking tool, related code should not be included in the final build (at least not automagically).

Maybe the same can be accomplished by moving it to the addon-test-support folder, but haven't tested this myself.

[bendemboski]

This also breaks embroider builds

[gilest]

Thanks for reporting. I've created a PR that simply moves those files into addon-test-support.

@bendemboski would you mind testing this with a static Embroider build?

I'll get the ember-try scenarios added soon.

[bertdeblock]

I noticed that ember-cli-mirage isn't in dependencies. Could that be related to the embroider issue?

[gilest]

I noticed that ember-cli-mirage isn't in dependencies. Could that be related to the embroider issue?

Yeah certainly could be. I think the idea in the past was to ship mirage route handlers with the addon in case users wanted to use them in application tests.

I'm not sure it's worth adding ember-cli-mirage to dependenices for this purpose. Perhaps the handlers could go in a separate package?

I'm trying to get CI/ember-try back up. Should have a better idea of compatibility then...

[jelhan]

I noticed that ember-cli-mirage isn't in dependencies. Could that be related to the embroider issue?

Yes. Embroider should break due to this. Embroider is more strict about explicit listing all dependencies, which are used. A package from ember-cli-mirage is imported here:

ember-file-upload/addon/mirage/index.js

Line 2 in 556075f

import Response from 'ember-cli-mirage/response';

If I didn't missed something it is the only place at which ember-cli-mirage is imported. Mirage's Reponse class is used at two places in the route handler:

ember-file-upload/addon/mirage/index.js

Line 49 in 556075f

resolve(new Response(408));

ember-file-upload/addon/mirage/index.js

Line 55 in 556075f

resolve(new Response(500, {}, { error: error.message }));

If I recall correctly the same can be achieved in Mirage without returning an instance of Response class. This would allow us to drop the dependency.

[jelhan]

Maybe the same can be accomplished by moving it to the addon-test-support folder, but haven't tested this myself.

The mirage handlers are needed whenever mirage is used. Often that is only the case for builds, which include tests. But there are also use cases to include mirage but not tests. Please see also my comment here: #427 (comment)

[bertdeblock]

I'm not super familiar with this addon and the reason it contains these mirage helpers, I assumed it was to mock during tests, but I didn't take into account mocking during development. So I'm aware my suggestion wasn't probably to best thing to do, it was only the first thing that came to mind when creating the issue 😄.

[jelhan]

The mirage handlers are part of the public API. They are not only used to test and develop this addon but also exposed to consumers to allow them to mock file uploading. This is covered in documentation here: https://adopted-ember-addons.github.io/ember-file-upload/docs/testing

[bertdeblock]

That's what I meant with mocking during tests and development yes.

[bendemboski]

If I recall correctly the same can be achieved in Mirage without returning an instance of Response class. This would allow us to drop the dependency.

Unless there's some mechanism I'm not aware of, it's not possible to provide a dynamic custom status code response (like ember-file-upload's mirage route helper does for the timeout and error responses) without returning a Response -- see this code -- you either return a Response or you just return the response body data, and it uses your handler's statically configured response code or falls back on the default response code for the handler's HTTP verb.

[jelhan]

Mirage had an option to provide a response code as third argument on route handler registration:

this.verb(path, handler[, responseCode, options={}]);

It is mentioned here in Mirage's documentation for 0.4: https://www.ember-cli-mirage.com/versions/v0.4.15/docs/advanced/route-handlers

I wasn't able to find it in documentation for Ember CLI Mirage >= 1.0. But Mirage seems to still have code related to it: https://github.com/miragejs/miragejs/blob/master/lib/server.js#L1035-L1060 I also wasn't able to find it mentioned in upgrade guides. Sadly the route handler registration methods seem to miss in API Docs as well. Long story short: I'm not sure if the functionality was deprecated or just missed to be documented for >= 1.0. 🤷‍♂️

[bendemboski]

@jelhan the issue is that the route handler dynamically returns different status codes for the success/timeout/other error cases. The other error case could probably be removed since mirage already has a catch handler that generated a 500 response, but unless we drop the timeout response functionality, we can't have the route statically specify a status code -- it has to return one dynamically which AFAIKT requires a Response object.

I have an idea that I'm testing out, which is to dynamically import the Response class using window.require() from inside the route handler. At that point we know mirage is running, so it should be safe, and shouldn't be subject to any static analysis that will mess up embroider.

[bendemboski]

I realize that I kinda hijacked this issue -- it does cause embroider build issues, but that's not what the issue is actually about, and my dynamic import fix for the embroider build doesn't address this issue, which is that ember-file-upload is shipping Mirage code into apps' production builds when that's almost never wanted.

So I can make that code build under embroider as a near-term solution for embroider compatibility, but we should probably stlll look into not adding Mirage code to the build when Mirage itself is not being added (and then the dynamic import solution would not be necessary).

IMO @bertdeblock had a smart idea with this comment, so I've opened an issue and corresponding PR with my proposed solution over in ember-cli-mirage. We'll see if they get any traction.

Perhaps we should file a separate issue tracking the embroider build failure, and either resolve it by merging #432, or port this solution to ember-file-upload and then wait for a resolution to miragejs/ember-cli-mirage#2166 to switch to a public API.

Thoughts?

[gilest]

Resolved in #567