Cli-microsoft365: Upgrade spfx 1.6 FN002008 saves exact when it shouldn't

[baywet]

When upgrading a project from spfx 1.5.1 to 1.6.0 the report instructs the following command:

npm i [email protected] --save-exact -D

However the default version specification for this package when scaffolding a net new project is ~5.0.0. I don't think this is a big problem, however if tomorrow 1.6.1 comes along and the cli implements the upgrade sequence as well, a project that was upgraded from 1.5.1 will trigger a finding for this rule.

[waldekmastykarz]

Why do you think this is the case? The package you mentioned above wasn't present in v1.5.1, so when you update the project to v1.6.0 it will suggest running the command you mentioned. If v1.6.1 still requires [email protected], then the rule will match the previously installed package and shouldn't yield a finding.

[baywet]

Thanks for the fast answer!
It makes sense, because the rule is only included in the version specific sequence, which is based on the version detected on the yo rc.

Allow me to correct my scenario:

• Upgrade a project from 1.5.1 to 1.6.0
• do no change the yo rc version
• re-run the upgrade sequence to make sure you didn't forget anything.
• this rule will trigger again because of the version specification difference

[waldekmastykarz]

Let me check if I can repro it 👍

[baywet]

side note, if this needs fixing, that probably means that #572 needs the "inexact" arrays again.
Let me know how this goes, I'll update my PR accordingly.

[waldekmastykarz]

I can't repro it. In my project I ran the upgrade from 1.5.1 to v1.6.0, ran the command as suggested by the CLI, and when I run upgrade again, the finding is not reported.

[baywet]

I'm still having the issue with the latest version that was just published.
Easier repro path.
Scaffold a spfx 1.6.0 project, edit the yo rc to 1.5.1 and run the upgrade command, you'll get the finding.
Technically you should not get any findings in this scenario.
Hope that helps.

[waldekmastykarz]

I'm looking at our code and we have tests that explicitly say we should return a notification even if the range matches the suggested package upgrade version, meaning this behavior is by design.

Imagine the following scenario: in package.json you have a dependency with version ~1.0.0. In our update rules, we'd say update to 1.0.1. Following what you'r saying, in this case we wouldn't suggest an upgrade, because 1.0.1 is covered already by ~1.0.0.

To get suggesting package upgrades more accurate, we could consider analyzing the lock file to see which package you have installed exactly, but that would mean redesigning the whole dependency checking mechanism which might not be worth the effort considering the alternative of running an extra npm i command. Once you run npm iwith the -SE argument, you won't be prompted of upgrade anymore.

[baywet]

So if I understand things properly the idea would be to make the version detection "more permissive" by considering ~5.0.0 or ^5.0.0 or 5.0.0 the same thing in the package.json? thus not triggering any finding.

[waldekmastykarz]

Not quite: what we do at the moment, is to detect if the specified version or the range satisfy the requirement coming from a rule. If the exact version equals the upgrade requirement we don't yield a notification. If the upgrade range meets the requirement, we yield a notification with the exact version just to be sure that you have the particular version which might be higher than the minimal version of the range.

[baywet]

That however doesn't bring fidelity with what a net new SPFX project with the newer version would be if I'm understand things properly? If the new project comes with ~5.0.0 and the cli instructs -DE (so 5.0.0), it technically makes a difference. The SPFX team intended people to get updates for that dependency and now we're locking it down. Don't you think a project upgraded with the CLI should be exactly the same as if I created a new project and brought my custom files over?

[waldekmastykarz]

By default, when you use npm, you're already getting a lock file, so even if you restore dependencies tomorrow, you will get the same version of each package as you installed initially. Without specifying the exact version to upgrade to, we couldn't update packages to newer versions if they're in the original version range, unless we manually rewrote package.json and instructed users to delete node_modules and their lock file, which could cause more trouble than installing a specific version of the package.

[baywet]

Yes, I'm aware of that :)
However omitting the ~ means npm outdated won't yield anything and npm update won't do anything. That'll make it harder for projects maintainers to know if it's lock down because it absolutely has to be that version or just because the tool the use didn't make the proper recommendation, making it harder to stay up to date.

[waldekmastykarz]

How would you propose we solve it?

[baywet]

we could create a inexact dependency rule that instruct users to enter npm i [email protected] -S, and then put back the different arrays I had in the part I was "compressing" the commands. I can take it on if you want.

[waldekmastykarz]

How would this rule deal with a case where package.json says ~5.0.0 but the latest version is 5.0.1 which meets the rule and which wouldn't trigger the upgrade?

[baywet]

you're right it wouldn't solve the issue. Besides a save (which translates by a caret), a save exact (which translated by the exact version number) and a range (which saves the range) there doesn't seem to be a way anymore to save a tilde in the package.json even if you specify it in the command npm i package@~5.0.0.
I think the only way to have full fidelity would be to build some kind of grep/replace command line which would replace the version spec in the package.json to exactly what's it's supposed to be in a new project. And then instruct a general npm install. I'm not super happy about that solution. Any other idea?

[waldekmastykarz]

With that approach, we'd also need to remove the lockfile, which would otherwise prevent parsing package.json. Given the trade-offs I'd say, let's not do it for now, even if it means that your project file is a bit different than if you created a new project.

[baywet]

Agreed, it's a lot of work for a tilde… Maybe we could add a description or something on that specific rule? suggesting people to do it manually?

[waldekmastykarz]

If you look back at the previous versions of SPFx, the same applied to the core packages as well. And it might get hard to get it fool-proof for a case like, in [email protected], all core packages were referenced by ~1.4.0. Back then, when you created the project, you'd get 1.4.0. If you installed the same project today, you'd get 1.4.1, which would give you a different project than what you'd get originally and that potentially might even not work.

[baywet]

One could argue that if they properly followed semver, a patch bump should not contain any breaking change. But I don't want to digress.
I think we can assume that most open source packages properly implement semver and a patch or minor bump should not be an issue. We should also be able to assume that if Microsoft didn't lock down that specific package, they trust that minor changes to it should not have a major impact to the broader SPFX.
Now to wrap things up for this discussion, we established that implementing any automatic control would be complex/costly at runtime. So I think our options are to either leave it as is or add some kind of message to let the user know that some manual change could be involved and that they are free to follow this guidance or not.
What do you think?

[waldekmastykarz]

Let's skip it for now. Let's keep an eye out on future updates. If we ever stumble upon any problems, we can reiterate this discussion and see what the best approach would be.

[baywet]

thanks for taking the time to discuss this item. closing for now

[waldekmastykarz]

Thank you for sharing your opinion. 👏