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

config.json file relative to process.cwd() #88

Open
ghost opened this issue Mar 3, 2016 · 8 comments
Open

config.json file relative to process.cwd() #88

ghost opened this issue Mar 3, 2016 · 8 comments

Comments

@ghost
Copy link

ghost commented Mar 3, 2016

All paths to config.json should be relative to process.cwd() otherwise when using cipherlayer as a dependency the config.json file is not found in the project root, instead you need to put it in the node_modules directory inside cipherlayer.

In the commit commit all references to config.json was changed to be relative to chiperlayer.

Is there a reason to not make the config.json relative to process.cwd() instead of cipherlayer? if not i can make a pull request to change them again.

@ghost
Copy link
Author

ghost commented Mar 3, 2016

This makes config.json location unknown until runtime, so IDEs can't locate/show the file. Also when we migrate to ES6 modules, imports and exports can't be dynamic.

As far as I know cipherlayer doesn't work as a dependency, right? If so, there should be a way to separate standalone and dependency.

If you make a PR, create a src/config.js and ONLY reference config.json there. Then, every file from cipherlayer should require src/config.js and not the json and encapsulate the config loading (standalone or deps) only in one place.

@luismesas
Copy link
Member

thats the same solution I was thinking on

@luismesas
Copy link
Member

in fact I was in the middle of that refactor when you did the lint one and then run out of time merging things... :)

@ghost
Copy link
Author

ghost commented Mar 3, 2016

I will make the pull request with that aproach

El jueves, 3 de marzo de 2016, Luis Mesas [email protected]
escribió:

thats the same solution I was thinking on


Reply to this email directly or view it on GitHub
#88 (comment)
.

Alejandro González

[image: Intelygenz] http://www.intelygenz.com/
t: +34 666 577 913 <34666577913>
e: [email protected]
w: www.intelygenz.com
[image: Google Maps] https://goo.gl/maps/LGNdq Pza. Sta. Mª Soledad
Torres Acosta 2. 28004 Madrid, Spain
[image: Twitter] https://twitter.com/Intelygenz [image: Google+]
https://www.facebook.com/Intelygenz [image: Google+]
https://plus.google.com/+Intelygenz-Innovation/ [image: LinkedIn]
https://www.linkedin.com/company/intelygenz Intelygenz. If you have a
dream, we can write the code

@ghost
Copy link
Author

ghost commented Mar 3, 2016

Ok I wait then 👍

El jueves, 3 de marzo de 2016, Alejandro González Rodrigo <
[email protected]> escribió:

I will make the pull request with that aproach

El jueves, 3 de marzo de 2016, Luis Mesas <[email protected]
javascript:_e(%7B%7D,'cvml','[email protected]');> escribió:

thats the same solution I was thinking on


Reply to this email directly or view it on GitHub
#88 (comment)
.

Alejandro González

[image: Intelygenz] http://www.intelygenz.com/
t: +34 666 577 913 <34666577913>
e: [email protected]
javascript:_e(%7B%7D,'cvml','[email protected]');
w: www.intelygenz.com
[image: Google Maps] https://goo.gl/maps/LGNdq Pza. Sta. Mª Soledad
Torres Acosta 2. 28004 Madrid, Spain
[image: Twitter] https://twitter.com/Intelygenz [image: Google+]
https://www.facebook.com/Intelygenz [image: Google+]
https://plus.google.com/+Intelygenz-Innovation/ [image: LinkedIn]
https://www.linkedin.com/company/intelygenz Intelygenz. If you have a
dream, we can write the code

Alejandro González

[image: Intelygenz] http://www.intelygenz.com/
t: +34 666 577 913 <34666577913>
e: [email protected]
w: www.intelygenz.com
[image: Google Maps] https://goo.gl/maps/LGNdq Pza. Sta. Mª Soledad
Torres Acosta 2. 28004 Madrid, Spain
[image: Twitter] https://twitter.com/Intelygenz [image: Google+]
https://www.facebook.com/Intelygenz [image: Google+]
https://plus.google.com/+Intelygenz-Innovation/ [image: LinkedIn]
https://www.linkedin.com/company/intelygenz Intelygenz. If you have a
dream, we can write the code

@luismesas
Copy link
Member

@guumaster can you explain that issu that will happen on imports to avoid it now?

@luismesas
Copy link
Member

we should solve this issue in a way that cipherlayer service gets the options from the container project on launch instead of searching them on a file

@guumaster
Copy link
Contributor

Well the import/export syntax would improve static analysis of the code and some other niceties. Its not actually an issue, you can always require instead of import.

But I've always thought that process.cwd() is a source of unknown behavior. And it can be easily replaced by an CLI argument on standalone launch or an option when requiring cipherlayer as a dependency.

I only know a very few packages that depends on process.cwd() like eslint, browserify or shelljs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants