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

Make pretender a peer dependency #1389

Closed

Conversation

jherdman
Copy link
Contributor

@jherdman jherdman commented Sep 30, 2018

Let's let the consumer of this addon decide which version of Pretender
is best for them.

Important Notes

  • I haven't gone to the lengths of supporting Pretender v2 yet. A preliminary spike suggests that's a bit more work than just bumping the version number. (Note: the problem seems to be that Pretender has a dependency on a few polyfill libs, and these either aren't being picked up or loaded correctly)

Questions

  • Mirage provides a shim for ES6 Pretender imports. Should that still be the case? If not we'll have to bump to much more modern Pretender.

@jherdman jherdman mentioned this pull request Sep 30, 2018
10 tasks
@jherdman
Copy link
Contributor Author

jherdman commented Oct 1, 2018

After looking a little more into this I think the peerDependency should be on ecli-pretender. What's not clear to me is how to "hoist" its work up into Mirage. I would love to pair on this with someone more knowledgeable, it's definitely exposing a gap in my understanding of ecli.

Let's let the consumer of this addon decide which version of Pretender
is best for them.
@jherdman jherdman force-pushed the make-pretender-a-peer-dependency branch from 0941758 to 9805c3f Compare October 1, 2018 13:17
@samselikoff
Copy link
Collaborator

Why ecli-pretender and not just pretender?

@samselikoff
Copy link
Collaborator

Would auto-import obviate the need for ecli-pretender?

@jherdman
Copy link
Contributor Author

jherdman commented Oct 1, 2018

The gist is that using Pretender means that you need to install a bunch of ancillary packages (see pretenderjs/pretender#221). It seems kind of wacky to duplicate this effort when @rwjblue has gone to the effort of doing this work for us in ecli-pretender. The challenge seems to be that nested addons are bit of a gong show. The gist is that you sort of have to "hoist" up some dependencies. This is the part I could use some help on.

@samselikoff
Copy link
Collaborator

I see. Ya this stuff is tricky... I personally don't know the right answer here

@jherdman
Copy link
Contributor Author

jherdman commented Oct 2, 2018 via email

@xg-wang
Copy link

xg-wang commented Oct 3, 2018

Hi @jherdman sorry I just noticed this from cross reference.
I'm not sure is it good to make it a peer-dep. Just want to point out ember-cli-mirage is loading assets here. To consume pretender v2, the only breaking change for mirage should be import whatwg-fetch (assume pretenderjs/pretender#235 merged).

@jherdman
Copy link
Contributor Author

jherdman commented Oct 3, 2018

@xg-wang many thanks for the response! Out of curiosity, why not make pretender a peer dep? It'd certainly give users greater control over which version of Pretender they could use?

@xg-wang
Copy link

xg-wang commented Oct 4, 2018

@jherdman IMO mirage is a higher level abstraction than pretender, it should hide its underlying impl and just provide its DSL for app. It should be mirage rather than consuming app's duty to take care of pretender.

@samselikoff
Copy link
Collaborator

@xg-wang In an ideal world I'd agree, however the reality is that I don't have the time or resources to make every Pretender API that Mirage users would find valuable available via a Mirage-specific API. I made the choice at the beginning to expose this.pretender for exactly this reason, and folks have used it for things like tracking requests when that feature landed in Pretender.

I think in OSS projects like this, this compromised middle road is the best path forward – exposing the underlying bits in a way that doesn't compromise the higher level APIs, so folks who get stuck behind a limitation of Mirage's APIs can dig down further if they need or want to.

Question: Is there a way to make Pretender a peerDependency of Mirage, but for Mirage to "supply" a version by default if the host app doesn't explicitly install it?

@happycollision
Copy link
Contributor

Could you not do something like the following? (And not make it a peer dep).

"pretender": "^1.6.1 || >=2.0.0",

@jherdman
Copy link
Contributor Author

jherdman commented Oct 9, 2018

@samselikoff I think a reasonable compromise in the mean time is to do the Pretender upgrade (see #1392), and continue to chip away at this problem in general. This would at least unlock fetch support in the interim.

@xg-wang
Copy link

xg-wang commented Oct 9, 2018

Sorry for the breaking change, I pushed another change to the pending PR in pretender. Link tested it won't break ember-cli-mirage tests.

@samselikoff
Copy link
Collaborator

After lots of back and forth, I think the best path forward today to solve this problem is to point people towards yarn resolutions.

npm peer dependencies are good in theory, but do not have widespread support among tooling layers nor adoption in our community. For instance, every addon technically has a peer dependency on ember-source, though none of them specify it.

Given the current state of things, dependencies seems to be the best place to put things that are conceptually peer dependencies.

If users want to override the version of a dependency like Pretender (or any other transitive dependency of their project), they can do so with yarn resolutions.

For more info check out #1497.

@samselikoff samselikoff closed this Feb 7, 2019
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.

4 participants