-
Notifications
You must be signed in to change notification settings - Fork 488
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 for collision of setModuleDefaults with get(), has() and friends #705
base: master
Are you sure you want to change the base?
Conversation
… friends. The problem is that defineProperty() defaults to configurable: false, which should only be false in makeImmutable().
This PR may also address a couple of the bullet points in #226 |
Anything I can do to help with getting this PR approved? I’m trying to remove a bespoke implementation of module defaults that was written for a project I work on. It’s mutually recursive (!), slow and memory hungry. This is the last bug I need fixed in node-config to remove that code in favor of setModuleDefaults() happy to explain or add more code coverage |
Giving this a +1, looks sane to me |
@jdmarshall In the related issue, you said you had a problem that was sometimes triggered and sometimes not. Why was the issue sometimes triggered and sometimes not? From looking at the patch here, either |
We have a very complicated use case, 3 apps that use some of our modules, a peculiar dev situation, an armful of CLIs, and several NodeJS versions. Some would work and some wouldn’t. That’s what I meant. This PR comes with a test that reproduces the underlying issue. |
Any progress on this? I’ve had to rename a bunch of config and add validation to disallow “get” as a key in our config files to prevent this happening to us. So many config files. |
I've been using a workaround for this issue for 13 months now. @markstos what needs to be done here? The original code misunderstands how defineProperty works in the absence of explicit settings, resulting in calls that do not accomplish what is desired. Then later attempts to define module defaults fail silently, which is the behavior when attempting to assign values to a property that is not defined as mutable. |
This PR addresses #689
The problem is that defineProperty() defaults to configurable: false, which should only be false in makeImmutable(). If you set it before that, you can't delete properties or modify the property.