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

devDependencies errors on ES6 #345

Closed
sylvanash opened this issue Sep 4, 2016 · 33 comments
Closed

devDependencies errors on ES6 #345

sylvanash opened this issue Sep 4, 2016 · 33 comments

Comments

@sylvanash
Copy link

I've used config before on a previous project and things worked great. I really like the library for it's simplicity and ease of use. It's like the underscore of configurations.
Anyway, I recently decided to create a new project based on the old one, but this time using ES6 syntax, that would then get transpiled to ES5 using gulp and babel.
Sadly, things didn't work out as I expected. Attempting to transpile the code or even to run it (using npm run start) throws an error that several modules were not found. After some inspection, I found out that these are ALL modules listed as devDependencies for config.
The devDependencies aren't installed using npm i (but I didn't expect them to)...but they also weren't installed in the old project (yet that still worked fine).
Any ideas on how to resolve this?

@lorenwest
Copy link
Collaborator

lorenwest commented Sep 4, 2016

This most likely has to do with conditional requires. The transpiler probably saw something like:

require('coffee-script');

and didn't know it was in an if/then conditional. Node config lists them as dev dependencies because any one project only needs one (or none). If all supported file formats were listed as dependencies, node-config would be too heavy weight for most people using it.

I don't have the same dev setup you do, so try changing all conditional require statements in lib/config.js to this:

var dep = 'coffee-script';
require(dep);

This may trick the transpiler into not having a breakdown if coffee-script isn't installed.

Let me know if that works for you. If so, we can make this change and it'll work for others that have a similar transpiling environment.

@sylvanash
Copy link
Author

That actually helped. config works fine now. Thanks

@markstos
Copy link
Collaborator

markstos commented Sep 6, 2016

Good intuition, @lorenwest !

@GuillaumeAmat
Copy link

@lorenwest It semmes that I have the same problem, do you plan to fix it?
Thanks

@GuillaumeAmat
Copy link

Oh crap no... It's me, my bad ;)

@markstos
Copy link
Collaborator

@sylvanash @GuillaumeAmat, I'm glad you got this working. Would you be willing to patch lib/config.js with the fix that @lorenwest recommends to help other people avoid the same problem?

@GuillaumeAmat
Copy link

@markstos My problem was not the same actually. I had imported node-config in a web app compiled by Webpack for a non-node.js target (the front end part).
So no fs module and so on...

@jonohodgson
Copy link

Any update on this issue?
After changing all conditional requires as per @lorenwest suggestion, I'm still getting
ERROR in ./~/config/lib/config.js
Module not found: Error: Cannot resolve module 'fs'

@lorenwest
Copy link
Collaborator

This hasn't been merged because it fails tests. I haven't investigated as it's generally the duty of the person requesting merge to resolve any failing tests.

@sylvanash
Copy link
Author

It worked for me after I changed the conditional requires, but I haven't
worked on that project in a while now so I don't remember much about it.
Will take a look and reply later with what worked for me

John .M. Amba
ash@circle.co.ke
+254703911024

On Thu, Nov 17, 2016 at 12:00 AM, Loren West notifications@github.com
wrote:

This hasn't been merged because it fails tests. I haven't investigated as
it's generally the duty of the person requesting merge to resolve any
failing tests.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#345 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIinCDU2Xu_CHmHw0zT3SLzYlaWQMKu8ks5q-279gaJpZM4J0bUO
.

@jonohodgson
Copy link

Thanks @sylvanash

@sorahn
Copy link

sorahn commented Jan 13, 2017

I was able to hack together something that works with webpack. It looks like this.

// webpack-config.js
const config = require('config')
const fs = require('fs')

// This will take the current NODE_ENV, and save the config to 'build/config.json'
// The webpack alias below will then build that file into the client build.
fs.writeFileSync(path.resolve(__dirname, 'build/config.json'), JSON.stringify(config))

module.exports = {
  // ... other webpack config
  resolve: {
    alias: {
      config: path.resolve(__dirname, 'build/config.json')
    }
  }
}

Both server and client now can happily use import config from 'config'. The client config is built in at build time, and the server is used at runtime.

@markstos
Copy link
Collaborator

@sorahn thanks for the example. The backend and frontend have an important difference when it comes to configuration: it's safe to sensitive values in the backend configuration and not in the frontend configuration. When you use the exact same configuration on the frontend and backend, how do you handle sensitive values securely?

Our approach to managing frontend configuration with node.js is to isolate frontend configuration in a "frontend" name space in the configuration, and then publish the frontend configuration at a /config.json URL.

@sorahn
Copy link

sorahn commented Jan 13, 2017

@markstos that is very true, and you could certainly keep a separate front end and back end configuration. As it stands right now the secret information the server needs is all passed in via ENV variables, or retrieved from a vault file on disk. Most of the configuration that we have in here is logging levels and api-urls and name spaces.

Also, good idea about making it 'json'. The code is cleaner that way :) . I'll update my suggestion.

Edit: You could also use something like lodash.pick to only write certain values to the front end config. It's nice to have the config be the same shape for a universal application.

@sorahn
Copy link

sorahn commented Jan 13, 2017

I added a page to the wiki here: https://github.com/lorenwest/node-config/wiki/Webpack-Usage

@lorenwest I hope that is ok. If not I will remove it.
(I didn't see a way to make a pull request for the wiki?)

@markstos
Copy link
Collaborator

@sorahn So then your "env" values and the vault data are not merged into the node-config object?

We also keep some values in ".env" files but use custom-environment-variables.json to merge them into the node-config object for convenient and consistent access by the backend.

@markstos
Copy link
Collaborator

I think having the Webpack example on the wiki is nice.

@sorahn
Copy link

sorahn commented Jan 13, 2017

@markstos no they are not. When the node server starts up it reads a vault file into memory and you can only access those variables through our sdk for the vault :)

import vault then vault.get('foo'). Has basically the same interface as config.

@lorenwest
Copy link
Collaborator

👍 You did the right thing @sorahn - thanks for creating that page. I'm sure webpack users will value it.

The only downside to wiki-only contributions is you don't get your icon on the project page. Add an example of webpack usage in a test and it'll get merged. Imagine bathing in that front page glory :)

@sorahn
Copy link

sorahn commented Jan 14, 2017

Haha OK, I'll look at getting it in a test too. Thanks!

@michaeljonathanblack
Copy link

@sorahn Great, your wiki helped me implement this! I updated your notes with some of my own:

https://github.com/lorenwest/node-config/wiki/Webpack-Usage

@sorahn
Copy link

sorahn commented Feb 4, 2017

Awesome! The system works!

One of these I will get around to investigating webpack plugins and just make one... One of these days...

jaredhirsch added a commit to jaredhirsch/ping-centre that referenced this issue Feb 6, 2017
`node-config` is hard to use with transpilers, because it uses
conditional dependencies, see node-config/node-config#345. Move
endpoint configs into a simple JS module.
jaredhirsch added a commit to jaredhirsch/ping-centre that referenced this issue Feb 6, 2017
`node-config` is hard to use with transpilers, because it uses
conditional dependencies, see node-config/node-config#345. Move
endpoint configs into a simple JS module.
emtwo pushed a commit to mozilla/ping-centre that referenced this issue Feb 6, 2017
`node-config` is hard to use with transpilers, because it uses
conditional dependencies, see node-config/node-config#345. Move
endpoint configs into a simple JS module.
emtwo pushed a commit to emtwo/ping-centre that referenced this issue Feb 9, 2017
`node-config` is hard to use with transpilers, because it uses
conditional dependencies, see node-config/node-config#345. Move
endpoint configs into a simple JS module.
@yvele
Copy link

yvele commented Feb 21, 2017

FYI I've done that to my Webpack config (targeting node):

{
  target  : "node",
  resolve : {
    alias : {
      "coffee-script"       : "empty-module",
      "iced-coffee-script"  : "empty-module",
      "yaml"                : "empty-module",
      "hjson"               : "empty-module",
      "toml"                : "empty-module",
      "cson"                : "empty-module",
      "properties"          : "empty-module",
      "x2js"                : "empty-module"
    }
  }
}

@subodhpareek18
Copy link

subodhpareek18 commented Feb 21, 2017

If wanting to use tools like create-react-app without ejecting means you can't really touch your webpack config, hence this https://github.com/lorenwest/node-config/wiki/Webpack-Usage probably can't be a complete solution.

Is there a possibility this can work out of the box, in the future if not right now? Or do you think there will have to be a change made in create-react-app repo itself to support node-config.

@sorahn
Copy link

sorahn commented Feb 21, 2017

@zusamann Great point about using create-react-app. I haven't built an app with node-config and create-react-app yet, but I do think we should be able to figure out a solution that doesn't involve ejecting.

@sorahn
Copy link

sorahn commented Mar 24, 2017

OK, I have an idea for this. With out changing node-config at all, we could write a wrapper library. That library could detect if it was being run in webpack. If so, it would just use the NODE_ENV from the webpack command to return a config so webpack could bundle it up. If we're running in node, then we just export node-config.

Thoughts?

@markstos
Copy link
Collaborator

markstos commented Mar 24, 2017 via email

@subodhpareek18
Copy link

Something like webpack-node-config does make sense, as it'd be the easiest solution.

But if possible I'd wish for npm i config that just works out of the box 😍!

@erichulburd
Copy link

erichulburd commented May 11, 2017

I came up with a different approach that avoids baking the config into the webpack build.

Basically, we can use a simple shim:

import { get, has } from 'lodash';

export default {
  get: key => (get(window.__CONFIG__, key)),
  has: key => (has(window.__CONFIG__, key))
};

(Obviously this would be easy to implement without lodash, but for simplicity's sake..)

Then in webpack:

resolve: {
  alias: {
    config: path.resolve(__dirname, 'src/client/lib/config.shim')
  }
}

Then on your server or static html file you would simply define window.__CONFIG__ before your build is loaded:

  <script>
    window.__CONFIG__ = <%- JSON.stringify(config) %>;
  </script>

If we wanted to do this without any additional webpack config, we could simply check typeof window === 'undefined' so we could export the shim in client environments.

@lorenwest @zusamann I'm happy to create a PR if this appeals.

@lorenwest
Copy link
Collaborator

I'm woefully un-equipped to evaluate this need, and I have no experience with webpack to determine if this is a reasonable approach.

Anyone else want to chime in?

As an alternative, an extension could be built and listed in our extensions wiki page, and if there's enough downloads over time, it would be a good candidate for inclusion in the core module.

@tnguyen14
Copy link

I'm bundling for the browser with rollup, and am running into the same issue of missing packages because they're devDependencies.

I think it makes sense to make the different file formats as sub modules within this, either as

import config from 'config/ts'

or

import {ts as config} from 'config'

so it'd work with import, and tree-shaking can help removing the packages that are not used, thus reducing the size burden?

@djaqua
Copy link

djaqua commented Sep 15, 2017

I came across this issue first and the solution that Sorahn posted (the "Webpack Usage" documentation) worked for me, as well. That said, immediately after trying to solve this issue, I came across the same sort of issue with Mongoose and solved it by installing and adding the node-loader to the list of loaders. Since that solved the Mongoose issue, I took out the "Webpack Usage" fix and it seemed to have solved this issue as well.

@markstos
Copy link
Collaborator

I'm closing this because there's been a lack of activity in the past 5 years.

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