Resolve the deprecate-early-static warning from ember-data #2444
Conversation
…ring the ED models via the modelFor method on the store, rather than statically requiring them (runspired lead me down this path)
app/initializers/ember-cli-mirage.js
Outdated
| @@ -16,6 +16,8 @@ const { default: baseConfig, testConfig, makeServer } = config; | |||
| export default { | |||
| name: 'ember-cli-mirage', | |||
| initialize(application) { | |||
| window.__app_for_mirage = application; | |||
There was a problem hiding this comment.
@kiwi-josh minor, I wonder if it might be safer to use a Symbol here, to ensure no one else uses __app_for_mirage even accidentally
There was a problem hiding this comment.
@SergeAstapov yeah good call out - I've updated the code to use a Symbol instead. Let me know if theres a better way you would like it done.
|
@kiwi-josh @cah-brian-gantzler this seem ok solution to me to avoid deprecation although I would like to consider some way to be able to explicitly access store service (don't have better suggestion wight now though) |
|
while this correctly resolves the deprecation (for now) I'd note a few things here
The best future-proof refactor here is for mirage to expose a hook and a format for supplying schemas. The easiest thing would be to align to the format the store's schema service currently provides today. |
… from consuming the value
|
I wrote a lengthy response to this and lost it :( give me a bit to re-write it. The TL;DR is, I do not believe this addon is required anymore (well, maybe the blueprints could stay) |
|
Wrote the reply as an issue. Thought that might be better found later. If this addon is kept, should be stripped down even more and some processes handled differently. Embroider has already raised issues about the way this addon discovers files, and all @runspired points are very valid. In order to prepare for the future, current patterns and process are going to have to be adapted |
|
As @runspired pointed out about discovering the data models, heres my actual situation My backend has many fields named differently than what I named them on the front end. I have ember serializers that rename the fields. Because discover models makes mirages models look like my front end models (mirage should look like your backend, thats what it is pretending to be), the fields are named wrong when mirage puts them in the payload. So I then had to create mirage serilizers to to rename the mirage field names into the backend field names so the front end serializer can rename them to the front end field names LOL. This was the main driver to me write the discover serializers (which was really a hack fixing a hack). TL;DR Auto discover was a convenience shortcut to avoid taking the time to do the right thing. When I first learned mirage I thought auto discover was awesome and for most people it works, but they do not actually understand that the tests are mis representing the back end. The first time you rename a field in a serializer you figure that out, but strangely there were never really any issues opened about it. I assume they created a mirage serializer to add a wrong to a wrong (which works, but didnt make it a right). (Three lefts do make a right though) |
|
I dont see a problem with merging the code to get rid of the deprecation. Long term though we have to do something to work with the changes that are going to be coming in Ember Data. While it is very nice that ember creates all the Mirage models for us, it does create them for every test even when you dont need them. Based on the last two comments from @runspired I feel this convenience crutch should actually be deprecated and users should be encouraged to create mirage models themselves. NOTE: for the last point, we need to document the way to use mirage in an addon that would only register models per test instead of all for every test. With the changes to how makeServer is called, we are all most there anyway. |
|
Sounds good. I'm looking forward to being able to point our app back at a stable version of ember-cli-mirage (instead of my fork) |
|
@cah-brian-gantzler sorry for the direct ping, but would it be possible to get a v2 release cut with this PR? Just to keep people moving forward with v2 without causing ember-data deprecations, and until v3 is stable and they do the work to which over? 🙏 |
|
@SergeAstapov what do you think? would you be able to do this? Work has me busy at the moment |
|
Kindly requesting a bump on this one 🙏 🙇 |
|
It would be really nice to get this solved. :-) |
|
Kinda lost track of this. Looks like the code was approved. We just need to merge it? The tests failed, but so long ago the logs are gone. Should we re-run the tests and see why? I think we have an issue with tests failing when they shouldnt and we need to revamp the CI pipeline. |
|
If it helps, we have been running this patch in production since september last year when it was opened with no issues |
|
Hi all – ember-data v5 has been released. Shall we re-run and see what's causing the CI failures? |
|
Hello 👋 . Any project using |
|
Hello. What could we do to help this PR get merged? |
- set `inverse` & `async` on models - upgraded `ember-cli`, `ember-data` and `ember-source` to `^5.0.0` - use [fixed version](miragejs/ember-cli-mirage#2444) of `ember-cli-mirage`
But instead apply to master
… but instead apply to master
- set `inverse` & `async` on models - upgraded `ember-cli`, `ember-data` and `ember-source` to `^5.0.0` - use [fixed version](miragejs/ember-cli-mirage#2444) of `ember-cli-mirage`
By discovering the ED models via the
modelFormethod on thestore, rather than staticallyrequire'ing them (@runspired lead me down this path as a resolution).Resolves: #2441
I've confirmed that the deprecation no longer appears for our app when running tests:
localhost:4200/testsyarn run testFor the record I don't think this PR should be merged in its current state - its more a POC to demonstrate there is a way to resolve this issue. Theres probably a much nicer way to get a handle on an application instance that doesn't involve a global on
window