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

Allow config path to be configurable #473

Merged
merged 1 commit into from
Mar 20, 2017

Conversation

bryanspears
Copy link
Contributor

@bryanspears bryanspears commented Oct 14, 2016

Our current packaging system requires a directory called "config" in the root. Was easier to do this than change the packaging system. Figured I'd contribute the change + a test since it doesn't hurt anything. 😄

@grawk
Copy link
Member

grawk commented Oct 14, 2016

I tried this change locally, along with renaming my config dir to fig:

app.use(kraken({configdir: path.resolve(__dirname, 'fig')}));

I receive the following error:

/Users/medelman/.nvm/current/bin/node server.js
Server listening on http://localhost:8000
Unhandled rejection Error: Cannot find module '/Users/medelman/src/krakex/gen-makara-dust-i18n-less/routes'
    at Function.Module._resolveFilename (module.js:325:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at index (/Users/medelman/src/krakex/gen-makara-dust-i18n-less/node_modules/express-enrouten/lib/index.js:15:14)
    at EventEmitter.onmount (/Users/medelman/src/krakex/gen-makara-dust-i18n-less/node_modules/express-enrouten/index.js:55:13)
    at EventEmitter.g (events.js:260:16)
    at emitOne (events.js:82:20)
    at EventEmitter.emit (events.js:169:7)
    at EventEmitter.<anonymous> (/Users/medelman/src/krakex/gen-makara-dust-i18n-less/node_modules/express/lib/application.js:237:8)
    at Array.forEach (native)
    at EventEmitter.use (/Users/medelman/src/krakex/gen-makara-dust-i18n-less/node_modules/express/lib/application.js:216:7)
    at register (/Users/medelman/src/krakex/gen-makara-dust-i18n-less/node_modules/meddleware/index.js:220:24)
    at Array.forEach (native)
    at EventEmitter.onmount (/Users/medelman/src/krakex/gen-makara-dust-i18n-less/node_modules/meddleware/index.js:199:14)
    at EventEmitter.g (events.js:260:16)

It's the same error you might see if you simply rename or delete the default config directory.

@tomalex0
Copy link
Contributor

@grawk Like to know if this is going to part of kraken framework soon?

@tomalex0
Copy link
Contributor

tomalex0 commented Nov 14, 2016

I have one more request specific to my use case, im trying to create sub application like
https://github.com/jasisk/middleware-patterns#subapp-pattern and would like to point all sub applications config to one folder .

More like an option for baseconfigdir or so.

@grawk
Copy link
Member

grawk commented Nov 22, 2016

My bad... user error. I tried to path in a fully resolved path in the config property. Oops. The change looks fine to me. Would be interested if any of the more deeply involved committers can see any possible drawbacks to this change? 👍 as far as I'm concerned.

@grawk
Copy link
Member

grawk commented Dec 23, 2016

would resolve #350

@bryanspears
Copy link
Contributor Author

Not hearing any more suggestions? We've been maintaining our own fork of kraken for months just to have this capability. :-)

@grawk
Copy link
Member

grawk commented Feb 16, 2017

I'm ready to merge this. Gonna find some more reviewers and knock this out. @bryanspears

@grawk
Copy link
Member

grawk commented Feb 27, 2017

@bryanspears want to doc this?

@grawk grawk merged commit 2bf2556 into krakenjs:v2.x Mar 20, 2017
@bryanspears
Copy link
Contributor Author

Looks like this was merged but never released? Just tried 2.1.0 via npm and this change is not present. Looks like nothing has changed on npm for kraken-js in two years?

What is paypal using these days? 😄

@gabrielcsapo
Copy link
Contributor

@bryanspears we are still using kraken! If it’s a testament to the operability of kraken it is that it hasn’t had to change in two years 😊

@tomalex0
Copy link
Contributor

tomalex0 commented Oct 23, 2017

Just checking if this change will be released soon :) ?

@gabrielcsapo
Copy link
Contributor

finally published https://github.com/krakenjs/kraken-js/releases/tag/2.2.0

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

Successfully merging this pull request may close these issues.

None yet

5 participants