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

What this addon does now that MirageJS has been extracted #2445

Open
cah-brian-gantzler opened this issue Sep 13, 2022 · 4 comments
Open

What this addon does now that MirageJS has been extracted #2445

cah-brian-gantzler opened this issue Sep 13, 2022 · 4 comments

Comments

@cah-brian-gantzler
Copy link
Collaborator

cah-brian-gantzler commented Sep 13, 2022

Quote from above #2444 will refer to it later
modelFor is also slated for deprecation. Though it likely will not be deprecated for 5.0, it will stop working if not using @ember-data/model which is slated to be removed from the default setup for 5.0>

  1. what mirage has done here has always been wrong. models have always been able to be registered dynamically.
  2. what mirage is doing will be even more wrong in the near future: there will be no models and dynamic registration of schema will be highly encouraged
  3. its often the case in apps you only want to register schemas for tests a single time, while in addons you often want to register schemas per-test

Once MirageJS was extracted to its own repo, the functionality in this repo was left to ember specific features. The way that this addon implemented mirage (for backward compatibility) did not align with the instructions and examples on how to use mirage in the MirageJS documentation.

Over the last year or so the implementation has been changed, exposing things like createServer, to more align the MirageJS documentation with the wya it is implemented in ember. There are more changes to go and this issue points out some of them.

All the documentation in MirageJS shows registering the models and routes when the server is created. The server configuration is not defined the same way for every test (as it is in this addon), but instead each test (or group if in a before each) defines the server with the routes and models that the test needs. (this refers to the quotes point 2 & 3)

What is left that this addon actually does

  1. provide blueprints for creating the correct files in the correct place
  2. provides a generic setupMirage (this used to do all the work, but now much of that work is done in config.js under the users control)
  3. provides a utility to auto create mirage models from ember data (creating the files manually is one time and low maintenance, but this addon does the work every time you run tests (its cached so only one time for all tests)
  4. auto imports the mirage models, factories, etc and creates a hash of them (suitable for being passed to createServer)
  5. provides an option to autostart mirage in a development
  6. provides a way of configuring the logging of mirage from qunit

let me know if I have missed any

Blueprints in point 1 I think are very helpful and definitely and ember thing and should stay

The generic setupMirage was originally built to do all the work behind the scenes with no user interaction. This is the main cause of the problem listed in the every point in the quote. To better align with the MirageJS docs, some of the work was extracted into the config.js function that now creates a server. This function receives a hash that implements feature 4 (which should be extracted to a utility that is called much like discoverEmberDataModels. Feature 3 (discoverEmberDataModels) has been completed extracted to a utility which is called in the make server if needed. setupMirage really just registers a beforeEach in which it creates the hash

Summary of what should stay in this addon and what should not

  1. Blueprints are an ember feature that I think might still provide value
  2. setupMirage is magic that isnt really needed anymore. Either create the server (as it is being done in config.js) in a beforeEach or the test. Import models, factories, etc (if you want to keep them in separate files (which means they do not have to be in the mirage folder anymore))(I think this plays nicer with embroider) or provide a utility method that creates the hash currently being passed to config.js when that hash is needed. This will solve the all the quotes points. setupMirage should be removed.
  3. auto discovering the models as it exists should be removed. Instead provide a utility that uses the container searching for "model:" and creates the mirage model config for the models that are registered. Call this after the test registers the models it wants. All solves all quote points. (This maybe applied to "factory:", etc, would have to look if mirage registers them with the container correctly).
  4. This could be provided as a utility much like discoverEmberDataModels. point 3 above
  5. remove this. If you want mirage started in development, do a create server with the routes and models you need. You can condition this start with embroider macros so that the code does not exist in production.
  6. I would keep this.
@runspired
Copy link

Autodiscovery is (period) bad. I think at most what ember-cli-mirage could do for ember-data is integrate with the schema service and build its models on-demand from the schemas that service provides. But really the mistake mirage is making here is twofold (1) in assuming a library like ember-data has models and (2) in using presentation classes (models) as schema sources (even for itself). Both of these are brittle foundations.

One of the points I was trying to convey in my comment is that the future of EmberData has no models. Neither in the container nor on disk. Autodiscovery simply will not work, because there is nothing to discover.

EmberData users will likely come up with a lot of ways of supply schema to the schema service (or be schema-less). The encouraged patterns will be around just-in-time derivation/loading. The friction here is probably because EmberData is not an ORM while Mirage attempts to be.

@hennii
Copy link

hennii commented Apr 23, 2023

@runspired I'm really curious about the idea of models going away from EmberData, do you have any more resources you can link to regarding this idea/discussion?

@cah-brian-gantzler
Copy link
Collaborator Author

@hennii I found this blog post a while ago https://blog.lux.name/2-dive-into-ember-data-1/ that is a good introduction. I havent found a part 2 yet, hope it comes eventually.

Did a lot of reading from @runspired overview https://blog.emberjs.com/ember-data-5-x-update-2023-04-15/ (Thanks for all the hard work), and am interested in references to how serializers are handled. I do need some property renames for example and not sure where that happens in the request/cache versions.

@runspired
Copy link

@cah-brian-gantzler any data munging to align API to cache format should happen in a handler. That said, such munging always should be considered a code smell if you have the ability to align the cache to the the API or the API to the cache. There are times when neither of these is possible (3rd party API with a format that's not very useful so you want a different cache format anyway) but they are usually rather rare.

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

3 participants