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

Detect @ember-data/model instead of ember-data meta-package #2355

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

jrjohnson
Copy link
Contributor

@jrjohnson jrjohnson commented Feb 10, 2022

While ember-data is usually installed it's also possible to use
@ember-data/model (and other packages directly). Since ember-data
provides @ember-data/model I don't think we need to detect both.

I'm not sure why this resolves to false in an app with either ember-data or @ember-data/model installed. I would expect true in both of those cases. I'm hoping @runspired or @ef4 might be able to shine some light on why this package may not work with the check in dependencySatisfies? I feel like maybe I'm missing something obvious, but I can't see it.

@SergeAstapov
Copy link
Collaborator

SergeAstapov commented Feb 10, 2022

@jrjohnson we have only ember-data listed in peerDependencies, probably we should add or even replace with @ember-data/model

@runspired
Copy link

What's the goal of this check? Probably safest to just check for the store package. The model package is optional for ember-data, the store is not.

@jrjohnson
Copy link
Contributor Author

I added (instead of replaced) it because I'm not sure about the DX of NPM/Yarn when they're missing. It might by confusing to be told to install the @ember-data/model when all the guides still talk about using ember-data.

@runspired as far as I can tell this is used specifically to detect the presence of possible models here so I went with the model package. I'm not sure if store would be a better choice though.

@jrjohnson
Copy link
Contributor Author

And that was the trick to making it work @SergeAstapov. I didn't even think to make it a peerDep here assuming if it was installed in the app then it would work. Which is the "something obvious" I was missing! This is ready for a serious look now, I have no opinion on what ember-data package we should depend on here. I could also add several of them?

While ember-data is usually installed it's also possible to use
@ember-data/model (and other packages directly).
@SergeAstapov SergeAstapov merged commit 189639f into miragejs:master Sep 13, 2023
12 checks passed
@jrjohnson jrjohnson deleted the detect-moar-data branch September 13, 2023 03:07
francois2metz pushed a commit to francois2metz/ember-cli-mirage that referenced this pull request Dec 19, 2023
Detect @ember-data/model instead of ember-data meta-package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants