Closed chrisforrette closed 7 years ago
This could work, I am still holding out hope that bulk-require will come up to date, but if you wanted to swap it out, and run tests I think we can do that and go live. I would like to see any security issues disappear from this project.
-- [image: Giftnix]
Brandon Copley
Founder & CEO
t: 512.784.6060 <(512)%20784-6060>
e: copley.brandon@gmail.com
On Wed, Apr 19, 2017 at 1:49 PM, Chris Forrette notifications@github.com wrote:
I'm working on a project that countryjs would be extremely helpful on, and we are using Snyk https://snyk.io/ to scan and block PRs that involve packages with security vulnerabilities and this one was flagged due to bulk-require https://github.com/substack/bulk-require being out of date.
There is a PR to fix this, which you seem to be aware of:
substack/bulk-require#11 https://github.com/substack/bulk-require/pull/11
...but it has been ignored for ~8 months, so in lieu of waiting for it to get merged and released, I was hoping to initiate a discussion on some work-arounds. I'm happy to put a PR together, but wanted to get consensus on the approach.
So my thoughts...
1.
Swap bulk-require out with another package. This one looks promising with some activity in the last couple months, no external dependencies, and is maintained by Felix, whose name I recognize from years of open source contributions: https://github.com/felixge/node-require-all The only point of concern for me is that I don't know how this affects the browser-side of things with bulkify. 2.
We could compile the list manually. I've done this before to get around dynamic require-ing on a project where Webpack was being used and it was very upset about it. This would look something like:
module.exports = [ require('afghanistan.json'), require('albania.json'), require('algeria.json'), // etc... ]
Not a glamorous approach at all, but countries don't tend to come in and out of existence a lot, so the maintenance should be simple, and with doing something like this before, I wrote a unit test that looped through all the files in a directory, excluding index.js, etc., and asserting their presence in the exported object. The tests will slow down a bit with this amount of data but it shouldn't be terrible.
Let me know what you think—thanks!
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/therebelrobot/countryjs/issues/57, or mute the thread https://github.com/notifications/unsubscribe-auth/ABJShvYrAW_ENgFGhOxb7VYIMvGTwXE2ks5rxlcpgaJpZM4NCFvd .
@BrandonCopley Cool—do you have a preference of approach? #1 vs #2?
miserable.
-- [image: Giftnix]
Brandon Copley
Founder & CEO
t: 512.784.6060 <(512)%20784-6060>
e: copley.brandon@gmail.com
On Wed, Apr 19, 2017 at 2:56 PM, Chris Forrette notifications@github.com wrote:
@BrandonCopley https://github.com/BrandonCopley Cool—do you have a preference of approach? #1 https://github.com/therebelrobot/countryjs/issues/1 vs #2 https://github.com/therebelrobot/countryjs/issues/2?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/therebelrobot/countryjs/issues/57#issuecomment-295413771, or mute the thread https://github.com/notifications/unsubscribe-auth/ABJShluq-4AmusjzZmF_aVHEpEezOQt5ks5rxmbxgaJpZM4NCFvd .
I'm working on a project that
countryjs
would be extremely helpful on, and we are using Snyk to scan and block PRs that involve packages with security vulnerabilities and this one was flagged due tobulk-require
being out of date.There is a PR to fix this, which you seem to be aware of:
https://github.com/substack/bulk-require/pull/11
...but it has been ignored for ~8 months, so in lieu of waiting for it to get merged and released, I was hoping to initiate a discussion on some work-arounds. I'm happy to put a PR together, but wanted to get consensus on the approach.
So my thoughts...
Swap
bulk-require
out with another package. This one looks promising with some activity in the last couple months, no external dependencies, and is maintained by Felix, whose name I recognize from years of open source contributions: https://github.com/felixge/node-require-all The only point of concern for me is that I don't know how this affects the browser-side of things withbulkify
.We could compile the list manually. I've done this before to get around dynamic
require
-ing on a project where Webpack was being used and it was very upset about it. This would look something like:Not a glamorous approach at all, but countries don't tend to come in and out of existence a lot, so the maintenance should be simple, and with doing something like this before, I wrote a unit test that looped through all the files in a directory, excluding
index.js
, etc., and asserting their presence in the exported object. The tests will slow down a bit with this amount of data but it shouldn't be terrible.Let me know what you think—thanks!