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

fix: add missing scope on flat options #4060

Merged
merged 1 commit into from
Nov 22, 2021
Merged

fix: add missing scope on flat options #4060

merged 1 commit into from
Nov 22, 2021

Conversation

yuqu
Copy link
Contributor

@yuqu yuqu commented Nov 18, 2021

The following command does not add scope into the npmrc file on npm cli versions >=7.7.0 as described in #3515

npm adduser --scope=@myScope --registry https://myArtifactory

Apparently 6598bfe refactors the config structure. flatten method of scope definition somehow sets flatOptions.projectScope but does not set flatOptions.scope. On the other hand, adduser command gets the scope parameter from the flat options, which is always resolving to undefined.
https://github.com/npm/cli/blob/latest/lib/commands/adduser.js#L28

I don't know if setting flatOptions.projectScope was a mistake or intended, hence I just kept it there. Alternatively, it could be renamed to flatOptions.scope instead of adding another line.

References

Fixes #3515

@yuqu yuqu requested a review from a team as a code owner November 18, 2021 16:02
@wraithgar
Copy link
Member

wraithgar commented Nov 18, 2021

Thank you for submitting this PR! projectScope was outside of config and consolidated in during v7.7.0 I believe. Ideally we would just change addUser to look for that config, but that would mean keeping a breaking change for existing users that have scope set in their config.

ETA: I was wrong, that wouldn't affect folks' having it set in their npmrc.

@wraithgar
Copy link
Member

wraithgar commented Nov 18, 2021

Question, what would prevent us from fixing adduser.js to look for projectScope in flatOptions instead? Saving it as scope in the config still for backwards compatibility.

@yuqu
Copy link
Contributor Author

yuqu commented Nov 18, 2021

projectScope was outside of config and consolidated in during v7.7.0 I believe

It seems projectScope is still there, outside of config https://github.com/npm/cli/blob/latest/lib/npm.js#L246-L255
It is the reason why I thought if there could be a mistake. Is it kept there for backward compatibility?

what would prevent us from fixing adduser.js to look for projectScope in flatOptions instead?

Right now flatOptions.projectScope is only set by scope in the config and there should be no problem if adduser.js also read it.

However, the code piece on the above link confuses me about whether it is the right call on to use projectScope in adduser.js. The difference is that, the "outside" projectScope can be filled by reading package.json file walking up the current directory when scope is not set on config. If flatOptions should be filled with this one, then I suppose there could be some unintended behavior when npm adduser is executed in a project tree.

@wraithgar
Copy link
Member

wraithgar commented Nov 18, 2021

the projectScope attribute that we attach to npm is not used anywhere and should be removed. It looks like we have more legacy behavior here that we can remove.

lib/utils/get-project-scope.js can be removed, test/lib/utils/get-project-scope.js can be removed, and lines 253-255 in cli/npm.js can be removed (as well as the require statement importing get-project-scope). The rest of that code block is supporting old configs that did not include the @ in their scope config, and won't affect what we're doing here.

Then lib/commands/adduser.js can save projectScope back to the config as scope

@yuqu
Copy link
Contributor Author

yuqu commented Nov 19, 2021

@wraithgar How does it look now?

@@ -1782,7 +1782,7 @@ define('scope', {
`,
flatten (key, obj, flatOptions) {
const value = obj[key]
flatOptions.projectScope = value && !/^@/.test(value) ? `@${value}` : value
flatOptions.scope = value && !/^@/.test(value) ? `@${value}` : value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still need to be projectScope because that's where npm-registry-fetch uses this to set appropriate headers in its requests

https://github.com/npm/npm-registry-fetch/blob/8954f61d8d703e5eb7f3d93c9b40488f8b1b62ac/index.js#L202-L203

@wraithgar
Copy link
Member

wraithgar commented Nov 19, 2021

MUCH better. Only one fix and that is the flatOptions attribute still must be projectScope.

Then lib/commands/adduser.js can save projectScope back to the config as scope

@yuqu
Copy link
Contributor Author

yuqu commented Nov 20, 2021

MUCH better. Only one fix and that is the flatOptions attribute still must be projectScope.

Then lib/commands/adduser.js can save projectScope back to the config as scope

I see that every mention of scope config throughout the project is named as scope, adduser.js, logout.js, auth/sso.js and their test files. I think there is no reason to rename it everywhere, instead switched back to the duplicate flattened options on my initial PR to be compatible with npm-registry-fetch.

What do you think? If it is not OK, I can do the rename thing. I'll rebase the fixup commit when I get your confirmation

@wraithgar
Copy link
Member

wraithgar commented Nov 21, 2021

It's important to note that when adduser, logout, et al are looking for scope it's in npm.config not in the flatOptions. The fact that it's named differently in flatOptions is a quirk of the way config was originally set up, trying to have a "flattened" config object that external modules got. It's not ideal and it's on our roadmap for dealing with when we redo config in npm@9.

Keeping it scope in the npm config is the right choice, and the only choice because otherwise we'd be introducing a breaking change.

For now I also think saving both to flatOptions is the right choice, and since ONLY npm-registry-fetch consumes that flatOption, we can make it look for scope once this PR lands. I've made an issue to track it: npm/npm-registry-fetch#59. It's also the right choice because it keeps the adduser.js using the scope flatOption, which once we fix npm-registry-fetch will be the only one used.

Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of unneeded code removed, my favorite kind of PR.

@yuqu
Copy link
Contributor Author

yuqu commented Nov 21, 2021

Rebased for better commit messages.

Opened another PR to fix the same issue on v7: #4073

@wraithgar wraithgar changed the base branch from latest to release-next November 22, 2021 14:49
@wraithgar wraithgar merged commit c5c6d16 into npm:release-next Nov 22, 2021
@wraithgar wraithgar mentioned this pull request Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm login/add-user --scope=@myScope do not set the scope on the .npmrc file
2 participants