-
-
Notifications
You must be signed in to change notification settings - Fork 862
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
Use parse-key-value-pair library #274
Conversation
Nice initiative 👍 I'm currently depending on both these modules in my project scandium. This would mean less redundant code for me to pull down, and I would be confident that the algorithm always are updated in sync 🙌 |
I don't know why appveyor failed? 🤔 |
lib/main.js
Outdated
value = value.replace(/(^['"]|['"]$)/g, '').trim() | ||
|
||
obj[key] = value | ||
const pair = parseKeyValue(line.trim(), { ignoreMalformed: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex will already trim out whitespace, so you don't need the .trim()
call
btw, with function parse (src) {
return buildObject(src.toString().split('\n').map(line => parseKeyValue(line, { ignoreMalformed: true }).filter(Boolean))
} might be a bit too condensced though 😄 |
Seems like a great idea! |
Glad the parsing code is going to good use. dotenv has had zero dependencies for a very long time. I'll defer to @motdotla on how important that is going forward. I am concerned about trying to support this module when all of the parsing logic is elsewhere in TypeScript (no offense to TS just not something I can support now or in the foreseeable future) edit: and don't worry about appveyor. see #241 |
Thanks! If you're afraid of the three typed lines I can put them in another file (index.d.ts) and have the source in index.js 😊 . It's compiled into that anyways 👍 I opted for TS since I wanted to use modern JS Why is zero dependencies a good thing? Including this library will reduce code duplication for CLI utilities and other things that also want to parse KEY=value and use dotenv. It will make sure the logic is exactly the same, reducing potential issues. But yes, I understand if you don't want to accept this patch 🙂 |
It may be helpful to have some discussion around why "zero dependencies" is desirable. I can see why few dependencies is desirable (less overhead in terms of install time & disk space; someone please add to that; front-end reasons like complexity to bundle do not apply) - and zero is the fewest possible - but maybe one dependency for this very good reason is justified (nothing like frivolously including all of lodash). |
So @motdotla and @maxbeatty would be given full access to the albinekb/parse-key-value-pair repo, correct? |
+1 on discussion, I can see from both sides of the argument |
@albinekb To be clear, what is the argument for (strictly) zero deps? |
@zenflow I don't think there's any reason to have zero deps, but I understand some people don't like deps, I love having small composable modules that share functionality 👍 For me it's a no-brainer. |
Yeah, I also don't see a reason for strictly zero deps, just for as few deps as possible. I look forward to hearing any reason for strictly zero deps. |
I am not a huge fan of this just because this is just about 50% of the functionality this lib is, we even expose it in a way that is reusable outside of the Moving this out to another lib I think makes us lose a lot of history and reasoning behind blocking or modifying our parser to handle different situations. I know it can be frustrating with the feature set not growing in this repo, but I think that has also been its strength. Small core, simple config. I would ask how do this benefit us as maintainers or the users of this module? |
I think (@albinekb correct me if I'm wrong) the benefit is as @LinusU stated above (#274 (comment)) - you can have both
Not sure I understand, and if I do, I don't agree. The But that is a great idea! What if we did that? Export a "pure" function that returns the parsed data. Then you could (essentially) have the benefit described above, but keep everything in this package; no need for a separate one. |
Love the initiative @albinekb. That said, a few things
You already can via
This is nice in theory but not in practice. The other repo would need strict coordination - in that when it had an update, dotenv would immediately receive the update. In real world scenarios where people are on different timezones and other issues are still open, it doesn't play out that way. The repos diverge for short periods of time - causing things to be out of sync. And finally, dotenv has built up a reputation of trust in the community. Part of that trust is because it is VERY easy to audit the code. Splitting this out to a separate repo would increase the potential attack surface in this regard. I'm sorry. Thank you for the PR but the costs here outweigh the benefits. |
@zenflow |
@jcblw Ah, so you're right. My bad, sorry, disregard my comment. I read the code wrong. Yeah, then I don't see the point of this PR since if you're already using dotenv you can just use |
Would you accept a PR that rewrote it in typescript? 😊 |
I would support exporting typescript types (or equivalent) if it doesn't require touching the JS. I'm more familiar with Flow and someone contributed types here |
Hi 👋 We're implementing some environment parsing options in a CLI and thought it would be nice to use the already battle-tested parser that's inside
dotenv
.I made a module out of it to make it cleaner, rewrote it in typescript for typings and added all tests from this repo: https://github.com/albinekb/parse-key-value-pair.