-
Notifications
You must be signed in to change notification settings - Fork 258
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
fix: don't require optional peerDependencies #2197
Conversation
✅ Deploy Preview for vue-test-utils-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
`@vue/server-renderer` and `@vue/compiler-dom` were both required in the sense that they will throw regardless of whether or not your are using functionality from those libraries if they weren't installed. I don't believe this was intentional, so I have made them optional and they will now only throw if you try to use them and the package isn't available, and will print a more helpful error message when doing so.
4212a60
to
e0585e4
Compare
How can I reproduce and test the issue this supposedly fixes? I am not comfortable merging this as it - I don't know what it fixes or how to test it. |
I'll see if I can create a small reproducer. |
I created a test repo that demonstrates 3 separate failures across yarn, pnpm and npm that are related to the issue in this PR. I was really hoping to just see the same problem manifest itself in the same way 3 times, but funny enough each package manager seems broken in slightly different ways. |
Today (after updating test-utils) I ran into the issue fixed by this PR. For now my workaround is to add hoist-pattern[]=@vue/server-renderer to my |
@thebanjomatic, do you believe that your pull request still necessary? @lmiller1990, @cexbrayat, could you say if the issue this pull request is trying to solve still valid? If the answer is not, I think we should close it to keep the repository clean with only real problems. |
I am not sure. I am not deep enough in the package management ecosystem to know, and I have never had an issue using Vue / Test Utils with any of the projects I'm working on. Since it looks like there isn't a pressing need for this and no-one else has raised issues, let's just close it for now. |
@vue/server-renderer
and@vue/compiler-dom
were both required in thesense that they will throw regardless of whether or not your are using
functionality from those libraries if they weren't installed. I don't
believe this was intentional, so I have made them optional and they will
now only throw if you try to use them and the package isn't available,
and will print a more helpful error message when doing so.