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

Play nice with transpilers by using variable requires (fixes #345) #358

Closed
wants to merge 2 commits into from

Conversation

blahah
Copy link

@blahah blahah commented Nov 11, 2016

As discussed in #345 this PR moves all conditional requires in lib/config.js to use a variable module name. This allows transpilers like Webpack to properly resolve the dependency graph.

@@ -883,7 +884,8 @@ util.parseFile = function(fullFilename) {
configObject = require(fullFilename);
}
else if (extension === 'iced') {
Iced = require("iced-coffee-script");
var dep = "iced-coffee-script"
Copy link
Collaborator

@lorenwest lorenwest Nov 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javascript hoists variables, and having individual var dep gives the misleading impression that these are different things, when they are all the same variable.

A better pattern would be to define var dep = undefined; at the top of the function and use that variable without (misleadingly) re-defining it in each instance.

Even better would be to have a block at the top of the file like this

// Define soft dependencies so transpilers don't include everything
var ICED_COFFEE_SCRIPT = "iced-coffee-script";
var JS_YAML = "js-yaml"
...

Then simply use these variables instead of the hardcoded strings require(ICED_COFFEE_SCRIPT);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to that. I blindly implemented the isolated solution given for one variable in the issue thread. I'll push an update now.

@jonohodgson
Copy link

I've taken this config.js but still getting
ERROR in ./~/config/lib/config.js
Module not found: Error: Cannot resolve module 'fs'

Is this working for others?

@blahah
Copy link
Author

blahah commented Nov 18, 2016

@jonohodgson I eventually discovered that it's extremely hard to get node-config working with Webpack (or any transpiler) in the browser environment in any meaningful way. This is because there is no fs module in the browser, and it's impossible to fully implement one.

In Webpack you can avoid that specific error using node: { fs: "empty" } (see the Webpack docs) which will give a no-op on any fs call. However, you will still face the problem that your config file will not be found by node-config, because it can't access the real filesystem. You can hack your config files into an in-memory filesystem polyfill, but at this point you may as well just have a config object in memory.

Ultimately we settled on exposing a shimmed CONFIG global using Webpack EnvironmentPlugin. An alternative that we considered but ultimately didn't need was node-config-loader.

@botagar
Copy link

botagar commented Sep 8, 2017

Hi. I was wondering what the progress of this PR is, and If I should create a similar one if this branch is not being worked on anymore.
This fix is something I need, and I would like the freedom to be able to clear my packages and re-install at will.

~~Cheers~!~~

Edit: Nevermind, I think I see the issue being face above. Sorry.

@gewentao gewentao mentioned this pull request Feb 2, 2018
@jdmarshall
Copy link
Contributor

This PR appears to be answered by #118

@markstos
Copy link
Collaborator

@jdmarshall #118 is an old issue about CSON support. Try again?

@jdmarshall
Copy link
Contributor

Yeah looks like they were referencing their own ticket.

#557

@markstos
Copy link
Collaborator

Given the comment about that says essentially "this wasn't going to work in the end anyway": #358 (comment) I'm closing this.

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

Successfully merging this pull request may close these issues.

None yet

6 participants