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

Enable rewiring rewired modules #78

Merged
merged 1 commit into from Nov 7, 2015
Merged

Conversation

TheSavior
Copy link
Contributor

babel-plugin-rewire, rewireify, and rewire-global all hook require, injecting the functions into every module.

When we use getDefinePropertySrc we run into problems on modules that pass through their dependencies unchanged. See this failing test on rewire-global: https://github.com/TheSavior/rewire-global/pull/13/files

The error we get is:

TypeError: Cannot redefine property: __get__
      at Function.defineProperty (native)
      at test/fixtures/keys-pass-through.js:7:8
      at Object.<anonymous> (test/fixtures/keys-pass-through.js:101:9)
      at Context.<anonymous> (test/tests/tests.js:63:5)

Before starting to use the upstream modules from rewire in rewireify and rewire-global, we got around this by setting the properties to writable: https://github.com/i-like-robots/rewireify/blob/master/lib/index.js#L37

Rewire does not set them to writable.

@TheSavior TheSavior changed the title Make properties writable Enable rewiring rewired modules Oct 13, 2015
@boneskull
Copy link

👍

@boneskull
Copy link

without this, rewire fails when running code with wallaby.js

cc @ArtemGovorov

boneskull added a commit to digsjs/digs-dev that referenced this pull request Oct 26, 2015
@TheSavior
Copy link
Contributor Author

@jhnns Can this get some love?

@jhnns
Copy link
Owner

jhnns commented Nov 2, 2015

Mhmm I'm not sure... @boneskull

In your example, wouldn't redefining __get__ or __set__ destroy the original getters and setters on the exported dependency? It looks like a small change, but I'm afraid that it might lead to subtle bugs when setters and getters are overwritten.

Usually, rewire() should always return a fresh instance, so you would not have this kind of problem. It becomes a problem because people started to re-implement rewire() for other environments only to some extent.

@TheSavior
Copy link
Contributor Author

It becomes a problem because people started to re-implement rewire() for other environments only to some extent.

@jhnns From your comments it seems like there is some misunderstanding between how all of our different rewire based packages work.

Your modules rewire and rewire-weback have the same use case. You can call rewire instead of require and then have access to the rewire methods.

rewireify, rewire-global, and babel-plugin-rewire have a slightly different usage pattern. Instead of explicitly calling rewire to add those methods, these modules add the functions to every module that is required via the standard require function. One reason to do this is to use the rewire functionality with proxyquire which also requires the user to use a different function than require. There are many other reasons as well.

So I think all of the module authors want us to all have the same API once rewire is enabled for a module, but we have different requirements for how rewire is injected into a module. In order for our modules to all be closer to rewire core, rewire-core needs to support both function injection patterns. I'm open to suggestions as to how you propose we get there.

@boneskull
Copy link

@jhnns regardless, rewire fails to work when run under wallaby due to this problem.

@ArtemGovorov could probably provided more accurate information if asked, but I think what's happening is:

  1. The user changes a test suite.
  2. The test suite is reloaded (rerun).
  3. The rewired module does not get reloaded (this partially accounts for the speed with which wallaby reruns tests).
  4. Result: The rewired module has already been patched by rewire, and cannot be patched again due to writable's default of false.

Unless it can be shown that setting writable to true actually causes a problem, this sounds a bit like FUD.

In case you disagree on philosophical grounds, I'm of the opinion that the responsibility of a JS library ends once it has done what it was intended to do. It is not necessary for any library to prevent a consumer from augmenting its functionality thereafter, except in rare circumstances. Perhaps this is especially so for rewire, which is intended to be run in test and augments its consumers' module(s)? If JS didn't allow this behavior by nature, rewire probably wouldn't even exist.

(FWIW, babel-plugin-rewire accepted a PR of mine to address this very issue.)

@ArtemGovorov
Copy link

@boneskull You've described it pretty accurately for babel-plugin-rewire. However, I don't remember anyone reporting issues with using rewire itself in wallaby.js. If it does fail, then it's probably for the same reason. If there's some sample I could run to reproduce the issue with rewire itself, I could provide a detailed overview of what is happening.

@boneskull
Copy link

@ArtemGovorov I don't have one handy, but I can provide an example if it crops up again

@jhnns
Copy link
Owner

jhnns commented Nov 2, 2015

@TheSavior that are all valid points. Maybe we should change the way how rewire() works. Maybe that's more pragmatically...

I'm just not happy that there are so many modules that do it "almost" the rewire way, because it can behave very differently in certain situations. It's just too much confusion for beginners.

Concerning this PR: I'm not quite sure... maybe I'll merge this PR as a quickfix. But it's not ideal because it obscures a conceptional problem underneath.

@TheSavior
Copy link
Contributor Author

@jhnns, I too am unhappy that rewire, rewireify, rewire-global, and babel-plugin-rewire are all slightly different. That is part of why I've been starting the discussions about how to make them all have the same api. This PR is a requirement that is blocking rewireify and rewire-global from using the same modules as rewire and thus having more shared functionality.

I think we want the same things here.

jhnns added a commit that referenced this pull request Nov 7, 2015
Enable rewiring rewired modules
@jhnns jhnns merged commit 23b79a8 into jhnns:master Nov 7, 2015
@TheSavior TheSavior deleted the writable branch November 7, 2015 21:20
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.

None yet

4 participants