-
Notifications
You must be signed in to change notification settings - Fork 169
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
Extract pretender code to its own module for future move to another package #1036
Conversation
Thanks for this work, @cah-briangantzler! I'm not terribly familiar with the pretender/mirage codebase (I mostly hopped in as a maintainer to keep dependencies and types up to date), but I really appreciate the work you're doing here, and I think it's a move in the right direction. I'd like to ask @samselikoff if he has any concerns with the process you've outlined in #1037, and if not, I say let's go for it. I'll give this a review as soon as I can, but I'm a bit swamped at the moment, so please ping me later this week if you haven't heard anything back. |
Thanks for the feedback. Yea, there definitely room for improvement but I wanted to open the ability to experiment (I already have MSW mostly working) without being a breaking change and this does that. There will be breaking in the future, trying to keep that to a minimum |
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.
Tests are passing and the code looks reasonable to me. Let's do this!
🎉 This has been published in https://github.com/miragejs/miragejs/releases/tag/v0.1.44. I know it might not have needed a release, but I figure it's best to take it slow, and get feedback from users if there are accidental breaking changes. Thanks again for your efforts, @cah-briangantzler! |
Doing a release is exactly what I wanted. Thanks My app has been updated to mirgejs 0.1.44 and running fine. Just one example though. Hope to get some feedback from others. |
@cah-briangantzler We're getting an error in one of our applications when adopting this update (but not in our other application - which is odd). Both applications are using
|
That is odd. The code as moved around should be doing the same thing. The main thing here is that it thinks you are creating a second pretender service which was never allowed. The pretender.shutdown is typically called via the afterHook from setupMirage. Is it possible a test is starting before another test has completed? Does this happen on the first test? or after several tests have run? If always the first test, I would look to see if autostart is somehow starting a server. (Hmm, think i removed autostart). I would put a breakpoint on the pretender start https://github.com/miragejs/miragejs/blob/master/lib/mock-server/pretender-config.js#L238 and stop https://github.com/miragejs/miragejs/blob/master/lib/mock-server/pretender-config.js#L529 to see where the second start is happening. The assert for the not changing the tracking request is here https://github.com/miragejs/miragejs/blob/master/lib/mock-server/pretender-config.js#L131. Thats original code and I dont actually understand what its trying to do. There is a trackRequests option you can do, but I have never done it. Does you second app set the trackRequests property and your first app doesnt? It may be that the track requests isnt handled properly in the moved code. |
Thanks for letting us know, @kpfefferle. If you're able to create a minimal reproduction of the issue, I think that would be the most helpful way we can dig into it, aside from the troubleshooting tips that @cah-briangantzler provided. |
I seem to run into the same issue as @kpfefferle after updating to v0.1.44. Unfortunately I don't have time to create a minimal reproduction at the moment. Maybe during the weekend. I've tried running a single test, creating a single server, without setting the |
@cah-briangantzler there have been several reports now of this issue. Do you have any time to look into it so we can fail forward, or should I plan to revert this PR until we can figure out what's going on? |
The track request code is pretty small. Yea, Ill add track requests to my config and see what happens |
@kpfefferle an update for |
I can at least confirm that it works for me now after upgrading to |
Thats great to hear, hope @kpfefferle will get a chance to try it soon. |
@cah-briangantzler I was on vacation last week, but I'll give the new release a try today and report back 🤞 |
Confirm that |
This PR moves all the pretender related code to the file
mock-server/pretender-config
and conditions the creation of pretender on the presence of an additional server optioninterceptor
.The goal of this PR was to be fully backward compatible. It opens up the ability to replace the interceptor with a different interceptor or allow for no interceptor at all.
First step towards #1013