-
Notifications
You must be signed in to change notification settings - Fork 439
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
Mirage as express middleware #1014
Conversation
44ff700
to
9139201
Compare
I see. Could you reach out to the FastBoot team and ask if there's a way to get babel-transpiled code into the Node environment? |
@samselikoff pls review - i think this is complete now |
Ok so - this depends on FastBoot? Can you walk me through it a bit. I wasn't thinking we'd need FB for this |
@samselikoff so issue we need to solve is loading ember code which normally runs in browser to Node.js env. Fastboot already solves this, so in a way it makes sense to build on that. These are the dependencies on fastboot:
Which dependency are you having issue with? I'd say We could try to do without fastboot, but that requires locating the Removing the fastboot dependency would not change the way this PR works it would only change |
Thanks for the response, I've been pressed for time but hoping to look at this in more detail soon. Conceptually my thinking was to read in the mirage code from disk and require it in node, and use it to define those route handlers within Ember CLI's existing express server. The reason to avoid adding fastboot or ember-cli-fastboot is just because they are more dependencies and deps can cause issues. Again I'll need to go through and see how they are used. What if we started with getting it working in Ember CLI's express but without automatic reloading? |
I've tried that route but mirage code depends on parts of ember which is not something you can just Otherwise we define the routes on Ember CLI's existing express server. Actually we define the routes on an
I hear you i'll look into getting it working without fastboot by doing the same eval stuff done by fastboot, but that change should affect small part of this PR. |
@mfazekas Ah I see - well Mirage was designed explicitly not to rely on Ember (for precisely this feature and future features related to node-land only stuff) so if there are Ember dependencies we should remove them. At the most I expect some utilities, no Ember.Object is used anywhere. Perhaps that should be a first step. |
@@ -167,7 +130,7 @@ export default class Server { | |||
let hasFactories = this._hasModulesOfType(config, 'factories'); | |||
let hasDefaultScenario = config.scenarios && config.scenarios.hasOwnProperty('default'); | |||
|
|||
this.pretender = this.pretender || createPretender(this); | |||
this.interceptor = this.interceptor || createInterceptor(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.pretender
was basically a public API, it let folks get access to the underlying pretender server. I'm thinking we should add code here to to create a this.pretender
alias if this.intercepter instanceof Pretender
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: createPretender
is now setting pretender on Server
.
We'll need a test for it though
https://github.com/mfazekas/ember-cli-mirage/blob/express/addon/interceptors/create-pretender.js#L48
app/initializers/ember-cli-mirage.js
Outdated
@@ -23,7 +25,7 @@ export function startMirage(env = ENV) { | |||
let modules = readModules(env.modulePrefix); | |||
let options = _assign(modules, {environment, baseConfig, testConfig}); | |||
|
|||
return new Server(options); | |||
return new Server(Object.assign({createInterceptor: createPretender}, options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use Lodash assign, since Object.assign doesn't work in Phantomjs
import _assign from 'lodash/object/assign';
_assign({ createInterceptor: createPretender }, options));
index.js
Outdated
@@ -112,9 +176,6 @@ module.exports = { | |||
}, | |||
|
|||
_shouldIncludeFiles: function() { | |||
if (process.env.EMBER_CLI_FASTBOOT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this was here so Mirage doesn't blow up when using fastboot. Does this no longer happen?
tests/integration/database-test.js
Outdated
@@ -1,5 +1,5 @@ | |||
import {module, test} from 'qunit'; | |||
import Server from 'ember-cli-mirage/server'; | |||
import Server from 'ember-cli-mirage/pretender-server'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be able to use ember-cli-mirage/server
and by not passing useExpress
they'll default to using Pretender?
index.js
Outdated
|
||
let emberCliMiragePath = __dirname; | ||
let mirageAddon = Funnel(emberCliMiragePath, {srcDir: 'addon', destDir: 'ember-cli-mirage'}); | ||
let appMirageConfig = Funnel(projectRoot, {srcDir:'mirage', destDir: 'mirage'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirage's dir can be configurable, we might need to look it up here... I kind of wish it wasn't though. shrug
Some comments reading through it. Excellent work on this, I'm so stoked for it to land!! Was wondering if we should ship it with 0.3 (which I was about to cut), or get it in for the last 0.2.x series. |
I think we should target |
ced991c
to
8faa142
Compare
That is very strange are we using lodash 3.x?? What's strange that using yarn it installs 4.x and i don't see issue on ember serve
|
I get the build error when setting |
I think the problem is the |
I was not getting the error when i used |
Gotchya. We'll need to get it working with npm as well. |
ping @mfazekas. we're so close! |
@samselikoff sorry i was busy lately with other stuff. So about lodash version, i'm a bit stuck it seems that with ember 2.12 projects are getting lodash 3.1* by default, and even if you add a package requiring lodash-4.* it will not be upgraded. With ember 2.13 a new project seems to get "4.17.4" even with npm. |
I'm also working on rebasing this to current version, and we'll need to figure out some automated way to test. Since we're compling to different env (node.js) it will be something easy to brake esp if new emberjs or other dependency is introduced. |
Not sure if it is known, but I just tried out this branch and got:
Hope it helps. |
@mfazekas there was just a lodash PR merged, perhaps it addresses the issue you were running into? I've been trying to focus mirage time on landing polymorphic associations so I haven't been able to dive into this. Also stanley was hacking on getting mirage running in a Service Worker, not sure if there are any implications for that here |
@samselikoff sorry i try to get some time to work in on the weekend. There is one issue that sometimes |
a potential issue with using lodash inflector is that custom inflection rules would not be properly inflected. |
@samselikoff The real reason this PR is stuck at the moment that ember-fastboot do have nontrivial ember depencencies. One example is inflector to pluralize, etc strings. While we can do some emulation with Another alternative is going back to drawing board and piggyback on fastboot, as fastboot let's Ember code run on the npm side. |
@samselikoff what's the direction you want to take with this PR? Or what needs to be done to have mirage supported for FastBoot? I am bit confused reading the entire thread. Happy to help with it. |
@kratiahuja this PR would make mirage work with Fastboot. But it's not tied to fastboot. There was a version where i did use fastboot to run mirage code on node.js, but in this version we're using broccoli directly to transpile mirage code to node.js. Ideally mirage would not have ember dependency at all, but it does have some use of Ember's |
Less than ideal is better than none. As far as I'm concerned, I'm really looking forward to seeing this PR land in Mirage. The benefits of this PR are fantastic! Being able to debug "as if" we were hitting the proper API and doing analyses with the Network Panel are really worth any tradeoff. Please land this 🙏 |
Hi @mfazekas, I tried to run this branch on a fastboot project(up-to-date) and got this error: |
Hello, is this work here going to be continued? |
Is there a new strategy that's being looked into for mirage + fastboot, @samselikoff and @mfazekas? I picked up this branch and rebased it with master and am working on getting it going with a project I'm using ember-cli-fastboot-testing on, but I'm running into some rough broccoli transformation errors. It looks like mirage now has fastboot as a dependency, should we revive the initial idea of using fastboot to do all that transforming? |
@jkeen I am going to be working on this soon (next month or tw) to help with the Fastboot testing story. Don't believe it's going to be Fastboot doing the transforming. |
I think Mirage as Service Worker sounds great. Would it be a better approach to modify |
@ming-codes good question. I don't know much about service workers yet. I think the APIs I'll be adding to get Mirage working in node will also make it work in a SW. |
I apologize for not communicating more around this issue & letting it linger. The reality was that this expands the problem space Mirage must support to a degree we weren't able to sustain with the resources we had on the project. Maybe this was my fault for not recruiting and empowering more folks to work on the project. In any case, we've picked this up for some related work, and FastBoot & Node support is a priority of ours at the moment. I'm going to close this PR for now since we're taking a slightly different path, relying on @mfazekas Thank you so much for kicking off this work and providing a place for folks to chime in, it helped us understand the need much more! Sorry again about letting this languish but hopefully we will end up with something that works for everyone soon. Anyone interested in the status of the feature can keep an eye on the FastBoot support issue. |
This PR adds the option to use express instead of pretender.
Usecases:
Implementation:
We compile
ember-cli-mirage
and app'smirage
dir to node.js using babel and borccoli. Sinceember-cli-mirage
usesember
some places we haveember-compat
that provides the used parts ofEmber
in node.jsthis is piggybacks onFastboot
, the main issue with is that current implementation of mirage runs in browser and we need it to run it onnode.js
. Fastboot already does that for the whole ember app. So we install an express middleware so that on first network request we can lazily create a Fastbooted (sandboxed) app instance, and we pass an express router to it. Then the sandboxed app can use the passed in express router to respond to real HTTP requests.It also depends on universal serving intruduced in ember2.12
apps needs to includeember-cli-fastboot
, so .js assets are compiled to node.js env.TODO
How to use
useExpress: true
in your ENV['ember-cli-mirage'] inconfig/environment.js
For example app see: https://github.com/mfazekas/fastboot-mirage-test/