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

Model Creation Side Effects #25

Closed
greaterweb opened this issue Jul 24, 2014 · 9 comments
Closed

Model Creation Side Effects #25

greaterweb opened this issue Jul 24, 2014 · 9 comments
Assignees
Labels

Comments

@greaterweb
Copy link

When running yo loopback:model a side effect of the model creation is minor updates to various json config files read by the generator.

package.json order of properties

My package.json file had the title property listed before the name property. After running yo loopback:model and adding a new model, the order was reversed.

newline at end of file removed

My projects leverage an .editorconfig file with the rule insert_final_newline = true. After running yo loopback:model and adding a new model, the following files had final newline removed:

  • package.json
  • server/config.json
  • server/datasources.json
  • server/model-config.json - other required updates to this file are made though the final newline is stripped in updating

While the changes seem insignificant, it will require attention when the files mentioned are in source control.

With the exception of model-config.json, is it necessary to write to these files?

@bajtos
Copy link
Member

bajtos commented Jul 24, 2014

Let's split this problem to two parts:

1. missing newline at end of file

This is IMO a bug we should definitely fix.

2. reordering of properties in package.json (and possibly other files)

This is caused by the internal design of loopback-workspace and how it represents various configuration files. I am afraid we can't preserve the ordering of items in JSON files in general.

However, it should be possible to detect the situation where the data was not changed (i.e. the canonical-json representation of what we have in memory and what is in the file yields the same content) and don't touch the file in such case.

@bajtos bajtos added the bug label Jul 24, 2014
@bajtos bajtos self-assigned this Jul 24, 2014
@bajtos
Copy link
Member

bajtos commented Jul 24, 2014

We are using jsonfile to write JSON files, I have opened a feature request to include EOL at EOF: jprichardson/node-jsonfile#13

@greaterweb
Copy link
Author

Thank you for tracking down the newline issue and raising a feature request so quickly with jsonfile.

Regarding the reordering of properties I understand why you are unable to preserve the order. I guess my confusion came from the fact the package.json did not have any updates to it after running yo loopback:model foo other than the reordered properties and missing newline at the end of file.

I briefly browsed the source for this project and loopback-workspace and was unable to easily determine where the package.json was being read/written to. I presume it's being read to check that one or more dependencies are met. Assuming that is indeed the case, could we limit the write condition to one that only happens if a dependency is missing?

Thanks again!

@bajtos
Copy link
Member

bajtos commented Jul 25, 2014

The truth is, the current implementation of loopback-workspace always loads and writes all files it knows about, regardless of whether there was any change in them.

could we limit the write condition to one that only happens if a dependency is missing?

I'd rather implement a generic solution where any JSON file write is skipped if the content in the file is equivalent to what we have in memory.

I am not sure if I manage to implement this today, before my two-weeks holiday. If you feel like contributing the change yourself, here is some info to get you started:

JSON files are written by configFile.save. The write should be skipped when the data was not changed.

A mockup of the change-detection algorithm:

function isChanged(filePath, data) {
  var cjson = require('canonical-json');
  var originalData = fs.readJsonFile(filePath);
  return cjson(newData) !== cjson(originalData);
}

@greaterweb
Copy link
Author

I may have time to explore a fixed based on your recommendations later this week.

@bajtos
Copy link
Member

bajtos commented Aug 12, 2014

FWIW, fs-extra@0.11.0 contains the change modifying writeJsonFile to add EOL at EOF, we should update package.json to use this new version.

@bajtos
Copy link
Member

bajtos commented Nov 25, 2014

@greaterweb is the reordering of properties still an issue for you? While loopback-workspace and yo loopback enforce order of certain properties, the order is changed only the first time you run the generator. Afterwards, the order "stabilises" and should not be changed.

@bajtos bajtos added #sprint60 and removed #sprint59 labels Dec 1, 2014
@greaterweb
Copy link
Author

It's less of an issue now, I really just noticed it in some of the initial work done using the generator.

Thanks for your efforts!

@bajtos
Copy link
Member

bajtos commented Dec 10, 2014

ok, I'll close the issue for now.

@bajtos bajtos closed this as completed Dec 10, 2014
@bajtos bajtos removed the #sprint60 label Dec 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants