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.

Sign up for GitHub

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 property files to be node modules that export objects #89

Merged
merged 1 commit into from
May 21, 2018
Merged

Allow property files to be node modules that export objects #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions lib/utils/combineJSON.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@
* and limitations under the License.
*/

var fs = require('fs'),
glob = require('glob'),
var glob = require('glob'),
deepExtend = require('./deepExtend'),
extend = require('lodash/extend');
extend = require('lodash/extend'),
path = require('path'),
resolveCwd = require('resolve-cwd');

/**
* Takes an array of json files and merges
* them together. Optionally does a deep extend.
* @private
* @param {String[]} arr - Array of paths to json files
* @param {String[]} arr - Array of paths to json (or node modules that export objects) files
* @param {Boolean} [deep=false] - If it should perform a deep merge
* @param {Function} collision - A function to be called when a name collision happens that isn't a normal deep merge of objects
* @returns {Object}
Expand All @@ -35,12 +36,14 @@ function combineJSON(arr, deep, collision) {
}

for (i = 0; i < files.length; i++) {
var file_content = fs.readFileSync(files[i], 'utf8');
var resolvedPath = resolveCwd(path.isAbsolute(files[i]) ? files[i] : '.' + path.sep + files[i]);
Copy link
Member

Choose a reason for hiding this comment

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

Is this module needed? Is there an issue with relative paths in require() on line 43?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the node resole alg. is basically (check me on this...)

if ./some/path.js or ../some/path.js start at the module you're in
if some/path.js resolve a node_module of name some and then the file path path.js

There's no part of it really that's "start at cwd() and give me file some/path relative to that".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could leave that up to the end user to make process.cwd() part of the glob pattern, too, I suppose.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I see now. It's a difference in how fs.readFile and require resolve the file paths, and because we are changing how we get the file contents from fs to require this is to ensue it is backwards compatible. Thank you for explanation and care to make sure this doesn't break backwards compatibility.

Copy link

Choose a reason for hiding this comment

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

the traditional method is to use fs.readFileSync(__dirname + './my-file')). The __dirname global is baked into node.js - https://nodejs.org/docs/latest/api/modules.html#modules_dirname

var file_content;

try {
file_content = JSON.parse(file_content);
file_content = require(resolvedPath);
} catch (e) {
throw new SyntaxError(files[i] + ' - failed to parse JSON: ' + e.message);
e.message = 'Failed to load or parse JSON or JS Object: ' + e.message;
throw e;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my outdated comment #89 (review) for an explanation. I think JSON error handling worked fine, but for all the kinds of errors that can be made in node modules the extra error detail is helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JSON error example with new... file name is in the existing e.message:

/Users/e828613/sc/manhattan/mds-tokens/node_modules/style-dictionary/lib/utils/combineJSON.js:46
      throw e;
      ^

SyntaxError: Failed to load or parse JSON or JS Object: /Users/e828613/sc/manhattan/mds-tokens/src/tokens/options/color.json: Unexpected token , in JSON at position 1816
    at JSON.parse (<anonymous>)
    at Object.Module._extensions..json (module.js:671:27)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at combineJSON (/Users/e828613/sc/manhattan/mds-tokens/node_modules/style-dictionary/lib/utils/combineJSON.js:43:22)
    at Object.extend (/Users/e828613/sc/manhattan/mds-tokens/node_modules/style-dictionary/lib/extend.js:113:17)
    at Object.<anonymous> (/Users/e828613/sc/manhattan/mds-tokens/src/dictionary-config/style-dictionary.js:1:117)

Copy link
Member

Choose a reason for hiding this comment

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

Perfect, thanks!

}

if (deep) {
Expand Down
Loading