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

Add support for requiring JSON files #4

Merged
merged 2 commits into from Jun 14, 2014

Conversation

Projects
None yet
2 participants
@shinnn
Copy link
Contributor

commented Jun 13, 2014

require can read pure JSON files, in addition to JavaScript files.
http://nodejs.org/api/modules.html#modules_file_modules
However, currently inlining-node-require cannot concatenate pure JSON files into JavaScript files properly.
So I updated lib/inlining.js to support JSON requiring and added a unit test for this feature.

Example

Sources
{"hi": "This is pure JSON."}
var data = require('path/to/json');
var greeting = data.hi;
Result
var data = {"hi": "This is pure JSON."};
var greeting = data.hi;
throw e;
}
});
try {

This comment has been minimized.

Copy link
@azu

azu Jun 13, 2014

Owner

better to add function like isJSON().

var trimedSrc = src.trim();
if(isJSON(trimedSrc)){
    return {};
}

or

var trimedSrc = src.trim();
return {
   src : trimedSrc,
   isJson : isJSON(trimedSrc)
}
[".js", ".json", ""].every(function (ext, i, arr) {
try {
src = fs.readFileSync(filePath + ext, "utf-8");
return false;

This comment has been minimized.

Copy link
@azu

azu Jun 13, 2014

Owner

Why not use fs.existsSync(path) ?

Addtionallly, I think better to separate this into functions.
like isSupportedFile and readFile.

ピンと来ないので日本語で、
サポートしてるファイルの種類なのか と ファイルを読み込む 処理が一緒になってしまってるように見えます。

それぞれを関数に分けて書くとクリーンな気がします。
(ただ、"サポートしてるファイルの種類"という定数的な情報は共有したいと思うので、ファイルを扱う小さなモジュールを追加してしまう等でもいいと思います)

また、fs.existsSync(path)を使えば、try-catchは必要なさそうに思えます。

This comment has been minimized.

Copy link
@shinnn

shinnn Jun 14, 2014

Author Contributor
  1. サポートしている拡張子の配列 supportedExts を定義する
  2. supportedExts をループして、実際に存在するファイルパスを返す getReadablePath 関数を定義する

という方法はいかがでしょうか。

var supportedExts = [".js", ".json", ""];
function getReadablePath(filePath, exts) {
    for (var i=0; i < exts.length; i++) {
        var pathWithExt = filePath + exts[i];
        if (fs.existsSync(pathWithExt)) {
            return pathWithExt;
        }
    }
    throw new Error (filePath + "not found.");
}
function inliningModule(filePath, entryData, opt) {
    // ...(略)...
    var readablePath = getReadablePath(filePath, supportedExts);
    var src = fs.readFileSync(readablePath, "utf-8");
@azu

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2014

@shinnn Thanks for pull-request. I have reviewed it.

@shinnn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2014

@azu Thanks for reviewing. I'll fix it.

@shinnn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2014

Done.

azu added a commit that referenced this pull request Jun 14, 2014

Merge pull request #4 from shinnn/master
Add support for requiring JSON files

@azu azu merged commit 9b7c689 into azu:master Jun 14, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@azu

This comment has been minimized.

Copy link
Owner

commented Jun 14, 2014

@shinnn Thanks. merged!

@azu

This comment has been minimized.

Copy link
Owner

commented Jun 14, 2014

Released 0.1.0.

@shinnn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2014

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.