[rush-lib] ChangeManger is overwriting existing files

[mathiasmaerker]

trafficstars

Summary

When invoking ChangeManager.createEmptyChangeFiles() twice or more with a time frame of 1 Minute (e.g. 12:01:01 - 12:01:59) existing files get overriden

Repro steps

The single invocations will override each other. Say you create a change file:

• at 12:01:01
• add your changes
• save it
• invoke ChangeManager.createEmptyChangeFiles() at 12:01:40again

your previously saved changes are gone, because they will get overriden.

Expected result: Minimal expectation would be that at least if a file is already existing, that path would be returned, better solution would be return the changefile

Actual result: files get overriden

Details

That is due to the fact how ChangeFile generates its name, see here

https://github.com/microsoft/rushstack/blob/0c26de5c017916b99603d2d50b511b4fcfa59270/apps/rush-lib/src/api/ChangeFile.ts#L77

and in ChangeManager you just call writeSync without checking if that file exists

https://github.com/microsoft/rushstack/blob/0c26de5c017916b99603d2d50b511b4fcfa59270/apps/rush-lib/src/api/ChangeManager.ts#L41

I anyway think you should return the ChangeFile instead of a plain string that way it would be way easier to interact with the files, and ChangeFile as far as I can tell, does not provide any critical internals, as a matter of fact, you only can getChanges addChanges and writeSync and you would expose that strange naming thing generatePath which would be a good thing imho, because for my use case I had to entirely reproduce it

Edit I see, that ChangeManager.createEmptyChangeFiles() is at least not used anywhere else inside rush-lib not sure if you use it somewhere else, but if not, that change should be easy

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question	Answer
Package name:	@rushstack/rush-lib
Package version?	5.62.4
Operating system?	Windows
Would you consider contributing a PR?	Yes
Node.js version (node -v)?	14.17.0

[octogonz]

This problem affects the rush change CLI command, not just the API.

Let's fix it! 👍

[octogonz]

From #contributor-helpline:

According to the source code, the current file name pattern looks like your-branch-name_2021-12-09-20-51.json and its uniqueness is based on the clock time YYYY-MM-DD-HH-MM. But it's not unique within the same minute.

One idea would be to change it to use a counter, like your-branch-name_2021-12-09-000.json. The 000 part would start at zero, and it would be calculated by examining the existing files and then choosing the first unused number. This sort of algorithm has well known performance problem in situations where lots of files accumulate (because the scan is O(n)), however that seems no likely given that these files get deleted regularly during publishing. I tried fs.readdirSync('.') on a folder with thousands of files and it was very fast.

[mathiasmaerker]

Hi @octogonz, sorry for my late reply, have been really busy. So basically you want to change the name generation in ChangeFile https://github.com/microsoft/rushstack/blob/0c26de5c017916b99603d2d50b511b4fcfa59270/apps/rush-lib/src/api/ChangeFile.ts#L77 correct?

But I am not sure this is the way to go, in rush change nothing gets overwriten asfaik. You check for file existence and append to the changelog. Do you want to change that as well? Just to clarify, in the end there would always be a single change file per change. no more appends etc. ? But that sound not like a Bugfix anymore, I fear that would change quite some logic in the underlying CLI? If you think otherwise let me know and I can try to work on that next week, I am on a Business trip and should find some time

Regards Mathias