embroider-build / addon-blueprint

Blueprint for v2-formatted Ember addons
MIT License
29 stars 26 forks source link

Remove yarn as default #145

Open NullVoxPopuli opened 1 year ago

NullVoxPopuli commented 1 year ago

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

simonihmig commented 11 months ago

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 https://github.com/ember-cli/ember-cli/pull/10368, but that logic has not changed. Can we regard this as a bug in ember-cli? /cc @bertdeblock @kategengler

kategengler commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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!