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

Add failing test case for rewiring an unused dependency: it should not throw #177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stuartsan
Copy link

Going back to this comment: #109 (comment)

@joshnuss You are right, rewiring a dependency which is not used or does not exist causes an error at the moment. I think we should be able to ignore such cases, as rewiring not existing dependencies won't change anything. @TheSavior what do you think?
Anyway @joshnuss could you please create a PR for a failing usage test? -- @speedskater

I'm running into this problem too, I'd like to be able to rewire something that hasn't been used yet without it exploding.

This PR adds a failing test case for the described use case, AFAICT this doesn't yet exist anywhere. If there's anything else I can provide please LMK. Happy to take a crack at a PR to fix as well if that would be useful.

@@ -0,0 +1 @@
const x = 10;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if you reference x anywhere else after this, the test will not fail.

@stuartsan
Copy link
Author

I can also add a case for not throwing when something does not exist, LMK.

@stuartsan stuartsan changed the title Add failing test case for rewiring an unused thing: it should not throw Add failing test case for rewiring an unused dependency: it should not throw Feb 8, 2017
@speedskater
Copy link
Owner

@stuartsan thank your for your effort and your offer of helping out with a fix. This would actually be great, as i won't be able to work on it before mid/end of march. Thank you and sorry for the inconvenience.
If you want to discuss this issue we can make a skype session.

@speedskater
Copy link
Owner

@stuartsan. Thanks for providing the test. I just had a look at your test.
And its not as easy to fix, as we need to know in advance, whether we rewired the module at all. This is the same issue as regarding wildcard in/exports.
We therefore decided to tackle all this in a new 2.0.0 branch. We also merged your test into this new branch, and will let you know as soon this is finished.

@stuartsan
Copy link
Author

Hey @speedskater , thanks! Apologies, I dropped the ball, meant to respond here. I had intended to try and fix this myself but poking around in the code I realized it was a major change as well and I was in a bit over my head. Thanks for the update and for all your work on this super valuable plugin, much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants