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

resolveAsyncConfigs currently "resolves" every character of a string #610

Open
1 task done
kristian opened this issue Jun 11, 2020 · 4 comments
Open
1 task done

Comments

@kristian
Copy link

kristian commented Jun 11, 2020

I'm submitting a ...

  • bug report
  • [ ] feature request
  • [ ] support request or question

What is the current behavior?

Currently node-config will attempt to resolve of config objects, using it's build-in resolveAsyncConfigs function. The function recursively iterates over object properties and array elements, in order to resolve them deep. Unfortunately even immutable strings, are currently treated as normal objects, and as strings can be character-wise accessed using [0] to [string.length -1], this behaviour even works. However this causes a massive performance drawback when "cloning" large string instances, when a simple "copy" could be enough.

What is the expected behavior?

String should not be treated for resolving. So in the resolveAsyncFunction a "typeof prop !== "string"" check should be added.

Also (in case you are just at it), there is i think a small syntax error in the util.extendDeep function, see line 1248:

if (mergeFrom[prop] instanceof Date) {
   mergeInto[prop] = mergeFrom[prop];
} if (mergeFrom[prop] instanceof RegExp) {
   mergeInto[prop] = mergeFrom[prop];
} else if ...

There is a "else" missing in this line! So I think it would be to change the if into:

if ((mergeFrom[prop] instanceof Date) || (mergeFrom[prop] instanceof RegExp)) {
   mergeInto[prop] = mergeFrom[prop];
} else if ...

Please tell us about your environment:

  • node-config version: latest
  • node-version: 11

Other information

(e.g. are you using an environment besides Node.js?)

@kristian
Copy link
Author

Forget it, I did a wrong call to setModuleDefaults which caused the behaviour! Sorry.

@kristian kristian changed the title extendDeep currently "clones" every character of a string resolveAsyncConfigs currently "resolves" every character of a string Jun 11, 2020
@kristian
Copy link
Author

Reopenign this, as I now behave the same behavuiour in resolveAsyncConfigs, this one will check every character of a string ;) So please apply the above fix to this function instead. :) Thanks!

@kristian kristian reopened this Jun 11, 2020
@markstos
Copy link
Collaborator

markstos commented Oct 6, 2021

Please add a failing test to the test suite which covers this case and submit your proposed change as a PR.

@markstos
Copy link
Collaborator

markstos commented Oct 6, 2021

@flodaniel this bug may be of interest to you.

@markstos markstos added the bug label Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants