-
-
Notifications
You must be signed in to change notification settings - Fork 473
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
Externalize @mswjs/* packages in certain builds #913
Externalize @mswjs/* packages in certain builds #913
Conversation
This comment has been minimized.
This comment has been minimized.
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 401e68c:
|
Let me know if you need me to clean up the history (squash) and / or let me know if you want me to handle the debug dependency problem in this body of work as well. Thanks! |
c9d5081
to
8b7f16d
Compare
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.
Hey, @patrickmcdougle-okta. Thank you for preparing this, the changes look great!
I've had a few comments and would like to know your opinion. I think this is almost ready to go, just a few things left. Looking forward to releasing this!
Regarding the debug
issue, let's address it in a separate pull request. I feel it may involve some changes in the biuld process of the interceptors library.
Sorry for the delay, I was off yesterday. I'll make the proposed changes today. Thanks for your patience and your review! |
8b7f16d
to
8536e86
Compare
I rebased to pull in the current tip of master, but kept a separate new commit for easy reviewing. Let me know if you need me to squash anything. |
The current CI fails on the
Seems related to #916. Edit: You can ignore this failure, it won't happen anymore. The issue was fixed by the underlying dependency that introduced it in the first place. |
Since we want to allow interceptors to have bug fixes without re-releasing msw, we need to externalize this module such that it is resolved in the consumers node_modules directory instead of bundled into this build.
8536e86
to
401e68c
Compare
I rebased again to pull in the current tip of master, but kept another separate commit for easy reviewing. Let me know if you need me to squash anything. |
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.
Thank you for this improvement, @patrickmcdougle-okta!
Collaborating with you on this was great. Let me know if you are interested in helping us with the project, there are always some tasks to work on.
Meanwhile, let’s have this one merged.
Sorry to bother on a closed PR...Will there be a release with this code included soon? It fixes a bug I'm seeing in a product I'm building and I'd like to avoid maintaining a fork. |
No worries. I'd like to include a few more bug fixes in the next release. There's no ETA, but I was planning on releasing the next version somewhere in October. |
+1 for releasing this. I can't use msw right now because of it. I described my use-case here #796 (comment) Thanks in advance |
@patrickmcdougle-okta @MikeYermolayev I'd recommend using the CSB build until the next release if you're in a bind. If you weren't aware, you can install as shown here: https://ci.codesandbox.io/status/mswjs/msw/pr/913/builds/169956 |
PR mentioned in #660 (comment)
Maybe fixes: #905
More work to be added possibly: #851 (comment) (which would likely fix #912)Not doing this here.