"[ERROR] SyntaxError: Unexpected token : " when using non .json file ending for config files

[bf]

./node_modules/db-migrate/bin/db-migrate -e circleci --config database.json.sample  up
[ERROR] SyntaxError: Unexpected token :
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:387:25)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at Object.exports.load (/..../node_modules/db-migrate/lib/config.js:22:14)
    at loadConfig (/..../node_modules/db-migrate/bin/db-migrate:117:12)
    at run (/..../node_modules/db-migrate/bin/db-migrate:319:7)

Using "version": "0.9.23" according to package.json with node 4.4.

It works again when I do cp database.json.sample database.sample.json and re-run the command with the new config.

I double checked the json file it is not malformed.

[wzrdtales]

@bf As the whole thing fails at require this does not belong to db-migrate, but to node js itself.

You would be able to reproduce this just by writing a single line of code yourself:

require('./data.json.sample');

And you would get your failure, if you change this and remove .sample everything works again.

So you may ask: What the hell is wrong with this? Well you just don't know yet how require works.

Require loads a javascript module and has also another mode. Loading json files, but json files are only, and really only parsed if the file is named with the .json suffix. In all other cases require asssumes your file is a file containing javascript.

[bf]

Didn't know about this undocumented require() quirk for specific file extensions. I tried it out and read through node source, and it indeed works as you said.

I think it would be more clean if you could name your config files as you wanted, and a simple fs.readFileSync and JSON.parse would do the same job.

From my perspective it was a "What the hell" moment because I thought require() is like PHP's include() which is a pretty dirty use to load a simple JSON config. Not sure how many of your users want to use .conf or whatever so this bug might be triggered here and there. Totally not expected that the file extension throws db-migrate off.

[bf]

Furthermore at least an error message should be added, and the db-migrate script shouldnt just crash with such an error message.

[wzrdtales]

@bf It is documented: https://nodejs.org/api/modules.html#modules_file_modules
https://nodejs.org/api/modules.html#modules_all_together

To citate:

LOAD_AS_FILE(X)

1. If X is a file, load X as JavaScript text. STOP
2. If X.js is a file, load X.js as JavaScript text. STOP
3. If X.json is a file, parse X.json to a JavaScript Object. STOP
4. If X.node is a file, load X.node as binary addon. STOP

As this is not anymore a question or bug report, you might want to open a new issue and open a feature request instead.

[wzrdtales]

Also to add, adding a custom error message to this would need to put try catch blocks around that require.

And also there is one thing that is against a fs.readFile + JSON.parse. No user would be able anymore to load a javascript file as configuration anymore. Because you could also create highly dynamic configs by requirering js configs which export the final configuration object.

Therefore, adding this feature would result in an unwanted behavior and a breaking change. There might be users that know about require and use filenames not ending with .js. So even if we consider to add to require non .js and non .json files with fs.readFile instead, we have to think about if it is worth to break the configs of many users.