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

jreichenberg · 2018-05-17T23:23:08Z

Issue #, if available: #85

Description of changes:
Allow property files to be node modules that export js objects, in addition to JSON files.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jreichenberg · 2018-05-17T23:39:33Z

... can now do things like this in a property file...

const myProps = {
  foo: 1
  bar: 2
};

myProps.sameAsFoo = myProps.foo;
myProps.somethingInYaml = loadYaml(...);
myProps.fromElsewhere = require('something.json');

module.exports = myProps;

dbanksdesign · 2018-05-18T04:32:05Z

Taking a look, but at first glance this looks pretty great! Thanks!

dbanksdesign

Looks good, just have a question on the resolve-cwd module

dbanksdesign · 2018-05-18T20:03:58Z

lib/utils/combineJSON.js

@@ -35,12 +36,13 @@ 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]);


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

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".

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

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.

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

jreichenberg · 2018-05-21T19:03:04Z

lib/utils/combineJSON.js

    } catch (e) {
-      throw new SyntaxError(files[i] + ' - failed to parse JSON: ' + e.message);
+      throw new SyntaxError(files[i] + ' - failed to load or parse JSON or JS Object: ' + e.message);


I noticed one other thing here.. by wrapping the e in a SyntaxError it loses some of the details about the actual error. The message is there, but accurate file and line number go missing. Any objections to just throwing the actual e here and maybe just maybe prepending the 'failed to load or parse JSON or JS Object' boilerplate to the actual e's message? The difference:

Throw the e:

TypeError: Cannot read property 'dimension' of undefined at tokens (/Users/e828613/sc/manhattan/mds-tokens/src/tokens/decisions/btn/btn-primary.js:4:40) at Object.<anonymous> (/Users/e828613/sc/manhattan/mds-tokens/src/tokens/decisions/btn/btn-primary.js:20:4) at Module._compile (module.js:652:30) at Object.Module._extensions..js (module.js:663:10) 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)

Existing (with a different but similar underlying error) doesn't have the detail in the stack...

SyntaxError: src/tokens/decisions/btn/btn-primary.js - failed to load or parse JSON or JS Object: Cannot read property 'none' of undefined at combineJSON (/Users/e828613/sc/manhattan/mds-tokens/node_modules/style-dictionary/lib/utils/combineJSON.js:45:13) 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:8:57) at Module._compile (module.js:652:30) at Object.Module._extensions..js (module.js:663:10) at Module.load (module.js:565:32) at tryModuleLoad (module.js:505:12) at Function.Module._load (module.js:497:3) at Function.Module.runMain (module.js:693:10) at startup (bootstrap_node.js:188:16)

Very good point, yes we should throw the e, but make sure the message for JSON files shows the filename. We redid the errors a few weeks ago to be more descriptive so it is easier to debug the issue, and adding the filename helps a lot (it previously just had a generic JSON error with no filename). So this fits perfectly in that theme of useful error messages.

jreichenberg · 2018-05-21T19:08:49Z

lib/utils/combineJSON.js

    } 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;


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.

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)

Perfect, thanks!

dbanksdesign

LGTM!

Dashue · 2020-04-02T16:23:10Z

@jreichenberg @dbanksdesign Was using json files and accidentally came across this pr.
Switched over to js files and wow what a difference in maintenance and development.

I thinkg the use-case with .js files deserves a better callout on the front page documentation. Would be invaluable to fellow developers like me coming along to use it.

dbanksdesign · 2020-04-02T16:27:01Z

@Dashue definitely! The doc site definitely needs some love, we will keep this in mind for the next iteration.

dbanksdesign reviewed May 18, 2018

View reviewed changes

jreichenberg commented May 21, 2018

View reviewed changes

Allow property files to be node modules that export objects

10331ae

jreichenberg force-pushed the issue-85-low-busget-fix branch from b0d7f21 to 10331ae Compare May 21, 2018 19:06

jreichenberg commented May 21, 2018

View reviewed changes

dbanksdesign approved these changes May 21, 2018

View reviewed changes

dbanksdesign merged commit 61c3062 into amzn:master May 21, 2018

dbanksdesign mentioned this pull request Jun 14, 2018

Issue running default "quickstart" template #98

Closed

chazzmoney mentioned this pull request Jun 30, 2018

Adding Yaml Example #84

Closed

dbanksdesign mentioned this pull request Aug 9, 2018

JavaScript config files #114

Merged

chazzmoney mentioned this pull request Sep 10, 2018

Transform after replace references #120

Closed

chazzmoney mentioned this pull request May 14, 2019

Reference modifiers #59

Closed

chazzmoney mentioned this pull request Aug 30, 2019

Aliasing objects at levels above value #205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jreichenberg commented May 17, 2018 •

edited

Loading

jreichenberg commented May 17, 2018 •

edited

Loading

dbanksdesign commented May 18, 2018

dbanksdesign left a comment

dbanksdesign May 18, 2018

jreichenberg May 18, 2018

jreichenberg May 18, 2018

dbanksdesign May 21, 2018

alex-h-strachan May 21, 2018

jreichenberg May 21, 2018

dbanksdesign May 21, 2018

jreichenberg May 21, 2018

jreichenberg May 21, 2018

dbanksdesign May 21, 2018

dbanksdesign left a comment

Dashue commented Apr 2, 2020

dbanksdesign commented Apr 2, 2020

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

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

Conversation

jreichenberg commented May 17, 2018 • edited Loading

jreichenberg commented May 17, 2018 • edited Loading

dbanksdesign commented May 18, 2018

dbanksdesign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbanksdesign left a comment

Choose a reason for hiding this comment

Dashue commented Apr 2, 2020

dbanksdesign commented Apr 2, 2020

jreichenberg commented May 17, 2018 •

edited

Loading

jreichenberg commented May 17, 2018 •

edited

Loading