Do not change order of fields in manifest when updating it

slavafomin commented 5 years ago

Hello!

Thank you for your hard work!

However, when Rush updates manifests (package.json files) it also changes the order of fields in them.

In our project we have a specific order of fields in all manifests and we enforce it using our special linting and auto-formatting tools, but Rush keeps re-formatting our manifests, which introduce phantom changes to our Git stage.

For example, we have peerDependencies before dependencies, but Rush puts them in reverse order.

Thanks!

octogonz commented 5 years ago

How do you describe your autoformatting rules? Would it make sense to configure Rush's autoformatting to manage this? (Longer term we are considering for Rush to generate package.json as a build output, in which case it might be more difficult for an external tool to manage.)

slavafomin commented 5 years ago

How do you describe your autoformatting rules?

It's a custom-build tool based on various npm libraries. For example, the order of fields is described as an ordered string array, where each element is a field name.

Would it make sense to configure Rush's autoformatting to manage this?

Is this an existing feature or are you proposing a solution for the future?

Longer term we are considering for Rush to generate package.json as a build output

Interesting, could you explain this a bit? I'm not sure I understand how this feature should work.

In my opinion, Rush (and any other dependency management software) is mostly interested in the list of dependencies and it's OK for commands like rush add or npm install to introduce changes to the list of dependencies. However, I firmly believe, that changes, introduced by package manager should be kept to a minimal, e.g. if you are adding a dependency, just add it to a list and sort the list itself, but don't change the order of root fields in the manifest. This will have minimal effect on the end-user code and will drastically reduce the chance of possible conflicts.

octogonz commented 5 years ago

Interesting, could you explain this a bit? I'm not sure I understand how this feature should work.

There are several problems that this could solve.

1. Multi-target builds

First, imagine that your publishing workflow produces three outputs, from the same source files:

my-package/package-rush.json - the file that humans edit; this does not get published
my-package/build/internal/package.json - this gets published to your company's private NPM registry, by running npm publish in the build/internal folder. This special release provides full access to APIs marked as @internal
my-package/build/beta/package.json - published with a version number like my-package@1.2.3-beta. The dependencies section will be rewritten to refer to beta versions. For example, if my-package/package-rush.json depends on my-library@1.2.3 it will get rewritten as my-library@1.2.3-beta when this file is generated by Rush
my-package/build/public/package.json - the official release with @internal and @beta APIs trimmed in the .d.ts rollup

The idea would be that a multi-target rush build would produce all three outputs simultaneously, writing outputs into separate subfolders.

2. Reducing Git merge conflicts

When running rush version in a large monorepo, suppose that someone has edited a core library such as a Guid class, which nearly every project depends on. If there are 200 projects in the repo, rush version may need to increment the versions for 180 of these projects (since they all use the Guid class). This shows up as a huge Git diff that churns the dependencies section for 180 package.json files.

This often leads to annoying merge conflicts. For example, in their PR, a human might make an edit like this:

my-library/package.json (human PR)

  "dependencies": {
    "library-a": "1.2.3",
    "library-b": "1.2.3",
+    "library-c": "1.2.3",
    "library-d": "1.2.3"
  }

That PR will encounter a merge conflict, because in the master branch, the publishing bot made an edit like this:

my-library/package.json (version bump from bot)

  "dependencies": {
-    "library-a": "1.2.3",
+    "library-a": "1.2.4",
-    "library-b": "1.2.3",
+    "library-b": "1.2.4",
-    "library-d": "1.2.3"
+    "library-d": "1.2.4"
  }

These merge conflicts could be avoided by storing the lockstep version number in a data file like common/config/rush/version-policies.json. And then using a token like this:

my-library/package.json

  "dependencies": {
    "library-a": "%MY_SDK_VERSION%",
    "library-b": "%MY_SDK_VERSION%",
    "library-d": "%MY_SDK_VERSION%"
  }

During the build, when Rush is writing the output file my-package/build/public/package.json, it would substitute %MY_SDK_VERSION% with 1.2.3 (or 1.2.3-beta). The versions are still getting churned, but the churn is no longer in files managed by Git. Thus we avoid a merge conflict.

3. Simplifying the source files

One other advantage of this process is that my-package/package-rush.json could be treated more like a source file: It could have comments, which are forbidden in a standard NPM package.json. And maybe it could use a relaxed input format like JSON5 or YAML. Common boilerplate (e.g. fields like license and scripts) could be inherited from a toolchain template.

slavafomin commented 5 years ago

Thank you Pete for a thorough explanation! These cases look pretty interesting.

So, you are proposing to introduce a special package-rush.json source file, which will be used to generate a package.json files, that will be published to npm, right? This indeed opens a door for a lot of functionality that could be build on top of it (comments could be especially helpful for complex manifests), however, such approach will break compatibility with other tools like npm itself and would introduce a vendor lock-in. I believe this could be a problem for some users.

Also, Rush can't be used as a 100% drop-in replacement for npm at the moment, so probably those missing features should be implemented first.

However, getting back to our use case and the issue in question I was talking primarily about editing the original package.json file. Considering it a source file we would like to control how it looks and how it's structured, but right now Rush makes it's own opinionated decisions about that when it introduces changes to it in automatic manner.

I would suggest for Rush to keep those changes to a minimum. I believe this would be a best strategy to maintain compatibility with other manifest linting and auto-formatting tools.

octogonz commented 5 years ago

however, such approach will break compatibility with other tools like npm itself and would introduce a vendor lock-in. I believe this could be a problem for some users.

How would this break compatibility? After you run rush install, your local project folders would still have a valid package.json file that should work fine with any package manager. The only difference is that it's not tracked by Git, so if you make any local modifications they would need to be copied into package-rush.json.

I would suggest for Rush to keep those changes to a minimum.

Which of these options would be most attractive to you:

Improve Rush's out-of-box normalization to be "good enough" for your needs
Provide a rush.json setting to disable all normalization performed by Rush
Add an event hook in rush.json, that allows Rush to optionally invoke your own custom script in order to normalize the file

octogonz commented 5 years ago

Let's use this issue to discuss these 3 possible options.

(I've split the "generating package.json as an output" idea into a separate issue https://github.com/microsoft/web-build-tools/issues/1512)

slavafomin commented 5 years ago

How would this break compatibility? After you run rush install, your local project folders would still have a valid package.json file that should work fine with any package manager.

I though that you were planning to replace package.json with the feature-rich package-rush.json. However, if you are going to keep both files (with duplicate content), this will result in awkward synchronization issues and again tools incompatibilities. I'm not sure if this is a good way to go.

Which of these options would be most attractive to you

If we are not talking about generating the custom package.json files now (which should be a separate opt-in feature I believe) then as I proposed earlier — package manager shouldn't be responsible for general manifest formatting.

Improve Rush's out-of-box normalization to be "good enough" for your needs

Yes, I believe, we should start with this. If Rush would introduce a minimum amount of changes (adding dependencies and sorting them, without modifying top-level fields order), this will definitely be a great base.

Provide a rush.json setting to disable all normalization performed by Rush

If you are still going to perform more complex auto-formatting on the manifest file I would rather have this feature disabled by default and have a flag to enable it. This would better mimic how npm works out of the box. Less suprises — simpler migration.

Add an event hook in rush.json, that allows Rush to optionally invoke your own custom script in order to normalize the file

This will be a great feature for our specific project. We have a local dev-dependency in all our packages, if Rush would be able to call it using CLI for each updated package, this will be very cool. Or if Rush will be able to execute a specific script from the manifest file, e.g. rush:manifest:update.

knownasilya commented 4 years ago

This also happens when using rush publish and fails finishing switching back to the master branch, and it's unclear if publish worked.

* EXECUTING: git checkout master 
error: Your local changes to the following files would be overwritten by checkout:
        apps/strider/package.json
Please commit your changes or stash them before you switch branches.
Aborting

ERROR: The command failed with exit code 1

matthiasg commented 4 years ago

rush add -p package also does not insert the package in alphabetical order, causes issues down the line when using rush version and rush publish

aruniverse commented 3 years ago

We've run into some issues with the publish step due to package dependencies not being in alphabetical order like Matthias has mentioned above. I was going to add a lint-staged step to sort package.json to catch this earlier using the sort-package-json pkg.

There is a way to specify the order of the other fields in the package.json, but without knowing the exact order rush expects it'll cause publish failures again. Could you guys provide a sample package.json with the right order expected? Here's how the sort-package-json orders it by default.

Also, this seems to be an issue for a lot of rush consumers, would this be something that rush can pickup on their end so individual consumers don't have to add this linting step on their end?

knownasilya commented 3 years ago

Also, this seems to be an issue for a lot of rush consumers, would this be something that rush can pickup on their end so individual consumers don't have to add this linting step on their end?

Yes please.

iclanton commented 3 years ago

We actually changed this behavior a while back to not sort package.json fields because it was causing issues: https://github.com/microsoft/rushstack/pull/1710

Since this has continued to be a problem for what seems like a lot of people, perhaps we should explore this proposal more: https://github.com/microsoft/rushstack/issues/1512, or we can pursue a simpler approach of having something like a /common/config/rush/package-structure.json configuration file that prescribes/enforces things like package.json field ordering. @octogonz - what are your thoughts here? Introducing some more configurability into the way Rush updates package.json files probably won't be too much work.

aruniverse commented 3 years ago

We actually changed this behavior a while back to not sort package.json fields because it was causing issues: #1710

Since this has continued to be a problem for what seems like a lot of people, perhaps we should explore this proposal more: #1512, or we can pursue a simpler approach of having something like a /common/config/rush/package-structure.json configuration file that prescribes/enforces things like package.json field ordering. @octogonz - what are your thoughts here? Introducing some more configurability into the way Rush updates package.json files probably won't be too much work.

@octogonz any update?

octogonz commented 3 years ago

Some more thoughts:

This problem is worth solving. In a large repo, it's pretty annoying when package.json files are not formatted consistently. When you have to do bulk updates across many files, it would be very nice for the dependencies to be sorted alphabetically.

Issue #1710 is a separate topic. I very much like the idea of a package-rush.json source file. But whether or not that gets implemented, we'd still want the package.json files to be formatted consistently.

Rush should provide a default solution. By default, the package.json files should be formatted consistently in a Rush repo. I'd like to see the same familiar package.json layout in every monorepo at work, without someone having to agree on a custom script/config and then roll it out to every team. Nobody has time for that (even though many people probably DO care about the problem). It doesn't need to be coded from scratch, if an existing NPM package like sort-package-json does what we need. But it should be built-in.

Rush should ALSO allow custom solutions. Rush should provide a simple "off switch" for strongly opinionated users who already built their own package.json normalization. That's an easy PR that we could merge very quickly if someone wants to make it.

Let's use #1503 for the "off switch".

I've created https://github.com/microsoft/rushstack/issues/2496 to implement a default formatting.

octogonz commented 3 years ago

The rush.json setting could be like this:

  /**
   * By default Rush will normalize the ordering and indentation of package.json files to make them
   * consistent between projects.  If you want to manage this problem manually or using some other
   * tool, set "preservePackageJsonLayout" to true. This causes Rush to preserve the existing layout
   * when updating these files.  The default value is false.
   */
  // "preservePackageJsonLayout": true

Any objections to that design?

microsoft / rushstack