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

Propose pnpm support #907

Merged
merged 3 commits into from
Jun 2, 2023
Merged

Propose pnpm support #907

merged 3 commits into from
Jun 2, 2023

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Mar 6, 2023

@github-actions github-actions bot added the S-Proposed In the Proposed Stage label Mar 6, 2023
@bertdeblock
Copy link
Member

bertdeblock commented Mar 6, 2023

I understand the desire to have ember-cli's pnpm support match its npm and yarn support, but personally I think that in the long run it's better to drop most / all of the package-manager related responsibilities from ember-cli.

This would include:

ember install (and thus default blueprints):

Back when npm was the sole / most used package manager, having the ember install command probably made sense.
Nowadays we have npm, pnpm, yarn, bun, ... They all come with their own API, and need to be detected in a slightly different way. For npm and yarn, we also have a few version checks in place.

I would rather see ember install (and default blueprints) go and teach users to just use their package manager of choice to install addons.

Running the lint:fix script post blueprint generation:

Slows down blueprint generation and doesn't always work reliably. It lint fixes the entire codebase and not just the newly generated files. Also, most teams probably already have tooling in place that makes running the lint:fix script unnecessary (prettify on save, prettify pre-commit, ...).

Auto-installing other addons and packages when running a blueprint:

Addons could / should define their peer dependencies in their package.json file.
I think npm and pnpm (v8) will auto-install peer dependencies by default? (if I understand pnpm v8's changelog correctly)

I think ideally, ember-cli (and the rest of the Ember ecosystem) works fluently under any package manager, but should try to remain agnostic about them as much as possible.

I think this train of thought is similar to other efforts in the Ember ecosystem where we try to get rid of Emberisms to align more closely with the wider JS ecosystem.

@simonihmig
Copy link
Contributor

I can somewhat relate to the arguments @bertdeblock brought up here. But that goes far beyond just dropping package manager responsibilities:

  • ember install with a default blueprint is also used to do more work than just installing dependencies, like adding/modifying in-repo files (example in ember-bootstrap). So dropping that is a tough call
  • we still need to have awareness of package managers when scaffolding new apps/addons, so we generate the appropriate files. Which also means we need to know how to install the dependencies based on that package manager, right?
    Side note: I assume supporting pnpm for this use case is actually Preston's main motivation here!? 😏

So unless we have an alternative RFC that proposes what @bertdeblock did and also tackles all those open questions, I think it is not justified to hold back this RFC based on this.

@MelSumner
Copy link
Contributor

MelSumner commented Mar 8, 2023

I support adding pnpm support but I don't want to remove the conveniences that Ember provides, so I agree with the what I inferred from @simonihmig's response --considering other things such as "should we really even have ember install?" are more appropriate for a separate RFC. Keeping the scope small here is, I think, a solid approach.

P.S. I really like ember install and lint:fix and while I have had some conversations to help me understand why some folks want to get rid of them, I hope we don't go that direction for the time being.

@bertdeblock
Copy link
Member

ember install is just a shorter way for installing an addon and running its default blueprint. Removing it would only mean that installing and setting up an addon would become more explicit. So pnpm add -D ember-bootstrap && ember g ember-bootstrap. ember g ember-bootstrap or maybe even ember g setup-ember-bootstrap would just be an additional documented step. This would result in an install flow that's package manager agnostic. I still feel that's the more future proof direction to take.

It feels difficult to me to answer the question: "Should we officially add pnpm support?" without questioning things like ember install, or using the project's package manager to automatically run a script. Because without things like that, I don't think we would need to ask the question in the first place.

To be clear, it's not my intention to hold back or block any RFCs, I'm just saying what I feel is the right thing to do.
I'm happy to support any outcome of any RFC.

@MelSumner
Copy link
Contributor

MelSumner commented Mar 10, 2023

Removing it would only mean that installing and setting up an addon would become more explicit.

@bertdeblock agree, I think that I like the UX of just typing ember install instead of the more explicit thing. I guess I'm all for adding the new explicit thing, I just still want the handy built-in shortcut.

Does adding support for pnpm mean that we would no longer be able to support ember install?


#### `--skip-npm`

The `--skip-npm` flag _actually means_ "skip installation of dependencies" when using `ember addon` and `ember new`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we propose a change of this flag to --skip-deps? Maybe that's separate but I can see it being related.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should -- I personally don't think that needs an RFC, (and is out of scope for this one) but I think it should happen.
RFC could then be put up for deprecating --skip-npm

- `ember install ember-resources` with `pnpm-lock.yaml` present will use `pnpm`
- `ember install ember-resources` without `pnpm-lock.yaml` present will use npm
- `ember install ember-resources --pnpm` will use `pnpm`
- `ember install ember-resources --no-pnpm` will use npm
Copy link
Member

Choose a reason for hiding this comment

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

What's the use case for this? If you had a pnpm-lock.yaml and didn't want to use pnpm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a consequence of our arg-parsing tool. boolean flags have equiv negated no-prefixed versions

- Are there any other references in the guides to `npm` or `yarn`?
The only place I could find that _mentioned_ `yarn` is here: https://guides.emberjs.com/release/addons-and-dependencies/
- Is there a `--package-manager` flag / option in `ember-cli`? for blueprint authors, that may be useful.
- Should `--skip-npm` be aliased to `--skip-install`?
Copy link
Contributor

@MelSumner MelSumner Mar 10, 2023

Choose a reason for hiding this comment

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

Ahh okay I see this part. Yes I think it should be aliased to something more generic. --skip-deps, --skip-install-deps, etc.

@wycats
Copy link
Member

wycats commented Mar 10, 2023

@NullVoxPopuli Should we generate an .npmrc for pnpm projects?

My (initial) preference is to generate one with:

auto-install-peers=true

I'm curious whether people have a preference around other settings. I'm also curious about any problems that might arise in practice with auto-installed peers.

@NullVoxPopuli
Copy link
Contributor Author

I'm personally against auto-install-peers, because it introduces surprises. I think folks should have correct package.jsons, and correctly declare which packages are needed by their direct dependencies.
auto-install-peers could be more convenient, yes, but for learning how dependencies work, I don't think it'd be useful, and I can see it being disabled entirely for serious projects that need to carefully manage their lockfiles.

@SolPier
Copy link

SolPier commented Mar 11, 2023

I agree with @NullVoxPopuli, but ultimately it is not for these reasons that I would not include the feature (generating a .npmrc with auto-install-peers).
To me the stronger argument is usage: waiting for the option to become popular. I assume it would even leak to other package managers at a certain level. Otherwise we should wait a bit, and let the meta decide where its going. Maybe auto-install-peers=false will become the norm ? There is no drawback in waiting for the dust to settle.

@runspired
Copy link
Contributor

Personally I suspect most addon installs should move to an executable remote command that isn't ember-cli based. e.g. npx <addon-name>

@MrChocolatine
Copy link
Contributor

I think npm and pnpm (v8) will auto-install peer dependencies by default? (if I understand pnpm v8's changelog correctly)

@wagenet wagenet added S-Exploring In the Exploring RFC Stage and removed S-Proposed In the Proposed Stage labels Mar 13, 2023
@ef4
Copy link
Contributor

ef4 commented Mar 17, 2023

Notes from discussion with core framework team:

Alternative ideas like "let's just drop ember-install" would be fine as followup separate RFCs, but that shouldn't necessarily block this RFC. In practice we already have support for pnpm in several places, and we have existing documentation about package managers, so improving consistency by officially documenting pnpm alongside npm and yarn is strictly an improvement, and doesn't limit our future options.

Before this RFC can move forward we probably do need to resolve some details around:

  • filling in exactly what the guides should say
  • getting consensus on auto-install-peers as discussed above

@NullVoxPopuli
Copy link
Contributor Author

Pnpm 8 just came out, and they decided for us, for auto install peers, no need for consensus.
https://github.com/pnpm/pnpm/releases/tag/v8.0.0

@NullVoxPopuli
Copy link
Contributor Author

Something @runspired mentioned, that we should probably change the default .npmrc for libraries / addons:

  • auto-install-peers=false (we don't want addons to be bad citizens of the ecosystem)
  • resolve-peers-from-workspace-root=false (we want true isolation, if a dependency is not declared, we want an error)

@wagenet
Copy link
Member

wagenet commented Apr 7, 2023

@NullVoxPopuli Do you know what you want to say in the RFC as far as guides or should we chat some?

@NullVoxPopuli
Copy link
Contributor Author

yeah, with how i'm thinking about this right now, I think chatting would be helpful <3

atm, I'm not sure what would be useful to say about pnpm, specifically (at least, in any way that would be different from how we handle npm vs yarn (which is already very minimal)).

I think the ember guides could use a section on general topics like:

  • dependency management (for apps and libraries, separately)
  • how to think about peers
  • the tradeoffs between widely used dependency managers

But that's something that should just happen (imo, ofc), independent of this RFC.

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented May 4, 2023

I've added a section about addons in the latest commit.

Hoping to make some progress on this RFC -- as I work on v2 conversions for @ember/test-helpers, @ember/test-waiters, and ember-qunit, it's looking less and less like they can stay on yarn@1 or npm1

Footnotes

  1. trying to be polite here, but like.. yarn just isn't going to work in a lot of situations with monorepos. It's the same for npm -- these tools have their place for non-monorepo projects, but with monorepos, they just don't work

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented May 12, 2023

Atm, I think all open questions / concerns are resolved, with the last question not directly addressed in comments:

Before this RFC can move forward we probably do need to resolve some details around:
filling in exactly what the guides should say

This is already included: https://github.com/emberjs/rfcs/pull/907/files#diff-116076a2d4078dfbf8de672a7b681ed1635715f3acfbfd31a1e7009251eef70dR134-R136

The explicitness is deliberately left vague, because a refactor of words is needed, as for example, in the CLI guides, this is the current phrasing:

Example use
This command below will create a folder called camping-trip-tracker, which will be full of app files. It uses the --yarn option to show that the app should use yarn by default. Yarn is a package manager alternative to npm. Options typically start with a double dash -- and can be omitted entirely.

ember new camping-trip-tracker --yarn

I think we'd probably want to update that to instead of being an "Example use" of the nearest heading, "Create a new app", to be "Specifying a package manager" with the existing text (tweaked a bit) being under a yarn sub-header, and then a pnpm under a similar header.

Personally I think this restructuring of this section of the docs is out of scope for the RFC, as the details such a change are more an implementation detail of "add equivalent pnpm mentionings wherever there are yarn mentionings.

Example of how I'm thinking about the update though:

## Creating an app

<default text, behavior, mention npm (cause default)>

### Using other package managers

npm is default, etc etc

#### yarn 

tweaked a bit from today's text

#### pnpm 

same as yarn text, but for pnpm, basically

#### using a non-official package manager

--skip-npm and do what you want

@wagenet wagenet requested review from a team May 22, 2023 15:54
@wagenet
Copy link
Member

wagenet commented May 22, 2023

This has been moved to Final Comment Period!

@wagenet wagenet merged commit 300304b into emberjs:master Jun 2, 2023
@wagenet
Copy link
Member

wagenet commented Jun 2, 2023

Accepted since we made it though the FCP period!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period S-Exploring In the Exploring RFC Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.