Add new entries on property object #356
Conversation
Introduce two new entries on property object: * `filePath`: the file path where the property comes from. * `isSource`: describe if the property is a source or an include. To set these two properties, the method `combineJSON` has now one more parameter `source`. This is a boolean which describes if json files are "source" or "include". By the way, `filePath` and `isSource` properties are removed from the original object properties (like it was in file).
| @@ -19,16 +19,26 @@ var glob = require('glob'), | |||
| path = require('path'), | |||
| resolveCwd = require('resolve-cwd'); | |||
|
|
|||
| function traverseObj(obj, fn) { | |||
There was a problem hiding this comment.
Do you think it would make sense to move this to its own file so we don't have to duplicate it here?
lib/utils/combineJSON.js
Outdated
| // Add some side data on each property to make filtering easier | ||
| traverseObj(file_content, (obj) => { | ||
| if (obj.value && !obj.filePath) { | ||
| obj.filePath = files[i]; |
There was a problem hiding this comment.
This should be obj.filePath = resolvedPath, that is causing some of the errors in the test.
| @@ -48,6 +58,15 @@ function combineJSON(arr, deep, collision) { | |||
| throw e; | |||
| } | |||
|
|
|||
| // Add some side data on each property to make filtering easier | |||
| traverseObj(file_content, (obj) => { | |||
| if (obj.value && !obj.filePath) { | |||
There was a problem hiding this comment.
I can't comment on lines you don't change, but we will need to change the line file_content = require(resolvedPath);. Because we are requiring the file rather than loading it with fs.readFile, any mutations we make to the object stay in memory, which is why some tests are failing and showing isSource as true when it should be false. We can fix this a few ways, first we could remove the && !obj.filePath so that it will always override tokens. Although this could cause weird issues in the future because we are mutating a module. Or we could change a line above from:
var file_content;
try {
file_content = require(resolvedPath);
to:
var file_content = {};
try {
deepExtend([file_content, require(resolvedPath)]);
Now mutating file_content would not result in mutating the original module. Or there might be some more elegant solution to create a deep copy of a module.
|
@7studio just checking in on this PR, have you had a chance to take a look at it? Let me know if you need any help! |
|
@dbanksdesign I am really sorry Danny, I fixed all the problems (all tests pass) before Christmas but I didn't take the time to push them. I rebased the branch with the last version of master and I hope I haven't forgotten something otherwise I will fix it again. I would like to congratulate you and your partner for your baby 👶 Good luck! |
|
@dbanksdesign Have I missed something about the PR not being merged since January? If I should apply some modifications, let me know |
There was a problem hiding this comment.
LGTM!
@7studio my apologies it has been a crazy few months. Our plan is to include this PR in a 3.0 release (along with some other core changes to the library). I just rebased the 3.0 branch so it is up to date and switch the branch for this PR to the 3.0 branch. There was a merge conflict because of another commit, but I fixed that. Going to go ahead and merge this in! Thank you for your contribution and patience!
Introduce two new entries on property/design token object: * `filePath`: the file path where the property comes from. * `isSource`: describe if the property is a source or an include. Co-authored-by: Xavier Zalawa <xavier@7studio> Co-authored-by: Danny Banks <djb@amazon.com>
Hi,
This is the PR to fix the issue #327.
Finally, I choose to follow Danny's comment about naming and tests problem and publish this PR.
It is not perfect but it's a good try I think 😉
I hope we will find the problem into
__tests__/extend.test.jstogether because my brain is stuck 😵Thank you for your help and your encouragements.