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

Remove yarn as default #145

Open
NullVoxPopuli opened this issue Jul 14, 2023 · 5 comments
Open

Remove yarn as default #145

NullVoxPopuli opened this issue Jul 14, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@NullVoxPopuli
Copy link
Collaborator

when using default options with this blueprint in particular, the package manager is switched to yarn.

when using --addon-only it's npm.

Let's remove the logic for setting up yarn by default to align with everything else and npm is default anyway

@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Jul 14, 2023
@simonihmig
Copy link
Collaborator

Let's remove the logic for setting up yarn by default

I can't remember any such logic, and also cannot find anything! Do you actually see anything like that?

When not passing --yarn, and you look at the generated CONTRIBUTING.md, you can actually see that it refers to npm everywhere! So we are picking npm as the default!

But you are right, when not passing --skip-npm, ember addon ... would call yarn to install packages. But that's out of our control, it's ember-cli doing this.

And it seems it is wrongly picking yarn as the package manager, when it finds a workspace definition in the root package.json, which is... weird? 🙃 Maybe still around from ancient times when npm was not supporting workspaces? 🤔

I see there was a major refactoring in ember-cli/ember-cli#10368, but that logic has not changed. Can we regard this as a bug in ember-cli? /cc @bertdeblock @kategengler

@kategengler
Copy link

I think it is a bug in ember-cli now that we support pnpm, but also fixing it will probably be a breaking change for anybody using yarn workspaces and relying upon that, so we'll have to think about how to fix it while still supporting that.

@simonihmig
Copy link
Collaborator

But any existing yarn user will have a lock file, so that detection function would still be able to identify an existing yarn repo as such, right?

@kategengler
Copy link

If you're in a workspace, do you have a yarn.lock at the lower level? I didn't think so and thought that was the purpose for the package used to detect workspaces.

@simonihmig
Copy link
Collaborator

If you're in a workspace, do you have a yarn.lock at the lower level? I didn't think so and thought that was the purpose for the package used to detect workspaces.

I see. So if that is how it should behave, then yes, we cannot just drop the logic and look for the lock file at the given directory only.

But that's actually not how it works! If you look at the implementation of find-yarn-workspace-root, it is traversing up the folder hierarchy until it finds a package.json with a workspaces definition, but it is not looking for a yarn.lock! So that would also match for a npm-based monorepo, which is why this is failing here I think!

That's different to what the@pnpm/find-workspace-dir counterpart does: it traverses the folders until it finds a pnpm-workspace.yaml, which is only true for pnpm projects.

I think we could use @manypkg/find-root instead, which will give us the correct root and the used package manager. If you have no objections, I could work on a PR next week!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants