-
Notifications
You must be signed in to change notification settings - Fork 128
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
Rewire seems to allow rewiring incompatible module types #147
Comments
Hi @brokentone 👋, thanks for reporting this 👍 I think your issue description is correct. rewire pollutes the Afaict, the only reasonable fix for that is to remove the Module wrapper hack and use pirates for this. |
Thanks for the response @jhnns! I personally believe that the current state is kind of the worst option -- not actually rewiring the module you intended, but actually rewiring a different one, and with the rewiring needs often being pretty subtle, it might be pretty hard to figure this out (thus my issue). I have proposed this PR: #149 in which minimally it makes that disconnect much more obvious. It doesn't fix the issue as such, but it makes it obvious that you have an issue. I imagine including this in a release would increase the support needs / issues filed, so it would be important to explain this clearly that "if you have X message now, you never were properly rewiring the named module" etc. For a solve, if you looped through all registered extensions and handled all of them -- or since you don't necessarily know how to handle particular files, perhaps there could be a singleton-y configuration step where users can add in other extensions? As a breaking change, the I like the proposal of migrating this logic to pirates as well. If you point me a direction (and ideally merge the PR I mentioned) I can contribute some. |
Update: rewrote the whole issue to attempt to better represent what I now understand to be the state of the system.
Just started playing with rewire (I know, welcome to 2015 or something) for the purpose of resetting a module's internal state with regards to
process.env.NODE_ENV
. The first thing I notice is that I can attempt to "rewire" all kinds (file types) I want, but sometimes there is no effect. This is confusing.After digging in, the issue seems to be that there is an imperative relationship between the module being rewired, and those being required. This means that when a module is "rewired" there are then 2 require loaders registered (
.js
,.coffee
). We then call require on the module, and assume that it will use one of the two loaders setup. This is generally an okay assumption in that.js
is the default, so long as we don't have a more specific loader setup, we're good. However, this is an issue when we use a non-standard file type (.ts
,.jsx
, etc) which ALSO has a loader, the imperative expectation is broken. AND we have left behind a side effect.There is absolutely no validation that this imperative contract is maintained -- that the loaders were called on the same file they were "rewiring." The impact of this bug is that we cannot modify the
const
s inside of these modules, but we do modify theconst
s of the NEXT js/coffee module which is required.I have a bug branch showing some of the oddities: master...brokentone:bug/rewires-wrong-module
I would recommend the following 2 improvements:
I am now opening a PR which attempts to address the 1st issue.
The text was updated successfully, but these errors were encountered: