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

this.addDevDependencies and this.addDependencies can improperly trigger conflict resolver #1299

Closed
ntucker opened this issue May 19, 2021 · 8 comments
Labels
needs triage Awaiting triage

Comments

@ntucker
Copy link

ntucker commented May 19, 2021

Just because a comma is added when addDependencies runs it creates a conflict and prompts the user in addition to breaking the install process

@ntucker ntucker added the needs triage Awaiting triage label May 19, 2021
@terenceodonoghue
Copy link

Running into this with yeoman-generator@^5.3.0. Can confirm that the trailing comma seems to be triggering the conflict resolver.

Screen Shot 2021-06-03 at 8 23 04 pm

@ntucker
Copy link
Author

ntucker commented Jun 3, 2021

@terenceodonoghue btw: one mistake I had was not awaiting these methods as they now return promises, which is why the conflict resolver was triggered.

This is still a problem for running yo on existing filesystems, but for completely new projects properly awaiting their return values should avoid this issue.

@terenceodonoghue
Copy link

terenceodonoghue commented Jun 4, 2021

@ntucker that fixed it, thanks! I replaced:

writing() {
  this.addDependencies([...])
}

with:

async writing() {
  await this.addDependencies([...])
}

Off-topic, but it's now failing for another reason: addDependencies and addDevDependencies attempt to resolve all packages against the public npm registry and ignores those I've defined in my .npmrc, thus the generator fails with a 404 error.

@mshima
Copy link
Member

mshima commented Jun 23, 2021

Closing, since it's resolved. The private repo problem is unrelated and should be discussed in the other issue.

@mshima mshima closed this as completed Jun 23, 2021
@ntucker
Copy link
Author

ntucker commented Jun 24, 2021

@mshima "This is still a problem for running yo on existing filesystems, but for completely new projects properly awaiting their return values should avoid this issue."

This conflict issue will appear if you have an existing package.json - which is often the case when you're using yo to add pieces to an existing project instead of just create a new one. (I assume this is the point of making yorc files)

@mshima
Copy link
Member

mshima commented Jun 24, 2021

If you want to force package.json to be written, you can do something like #1239.
Or write a .yo-resolve file with

package.json force

@ntucker
Copy link
Author

ntucker commented Jun 24, 2021

Why is there a merging policy if the expectation is to only write to existing files if it's completely forced?

@mshima
Copy link
Member

mshima commented Jun 24, 2021

What are you spectating here?
To merge? To don’t generate at all? To use existing values?

The default is to merge, which is the better default for composing.
If you want to keep old values as default, you should load them.

this.addDependencies({
  …dependencies,
  …this.packageJson.getAll().dependencies
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Awaiting triage
Projects
None yet
Development

No branches or pull requests

3 participants