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

Switch default package manager to pnpm for new projects + C.I. #820

Closed
NullVoxPopuli opened this issue May 18, 2022 · 44 comments
Closed

Switch default package manager to pnpm for new projects + C.I. #820

NullVoxPopuli opened this issue May 18, 2022 · 44 comments

Comments

@NullVoxPopuli
Copy link
Sponsor Contributor

NullVoxPopuli commented May 18, 2022

pnpm is rock solid, and way faster than everything else.
For new projects I clone to my machine, I can have pnpm install start and finish in under 3s and then I can proceed with ember s

pnpm also has correct handling of peer dependencies, and has solved every problem I've ever had with npm and yarn both with the module resolution algo, as well as monorepos (I also do not have a high opinions of npm workspaces, and even lower opinion of yarn@v1 workspaces).

Thorough benchmarks: https://pnpm.io/benchmarks

Thoughts?

I'm also of the opinion that we should probably remove yarn support if we don't upgrade it to yarn@v3, because yarn 1 is .... terrible (iirc, they themselves haven't been supporting it for ages, and (again, iirc) they even acknowledged it had so many bugs, they just started over)

@jrjohnson
Copy link
Sponsor

jrjohnson commented May 18, 2022

I have absolutely no idea what the best package manager is, honestly I don't care too much and mainly use npm because it is the default and one time Yehuda said it was fine on twitter 😁 .

I would really like it if there was a single default choice for ember though. Switching between npm and yarn working with different addons has always struck me as unnecessary friction and as we move to v2 addons which seem to be best built as a mono-repo with different test apps instead of a single dummy app I think making an explicit default choice would be a big help in documentation and community momentum and remove some of the difficult to read and reason about branching in the ember-cli blueprints.

If pnpm is awesome and well supported and works in embroider future I'm all for making this official, if we'd be better off forcing some version of yarn or npm that's fine with me too.

@jherdman
Copy link

For the uninitiated, why should ember-cli have an an opinion about which package manager I'm using at all?

@NullVoxPopuli
Copy link
Sponsor Contributor Author

NullVoxPopuli commented May 18, 2022

Switching between npm and yarn working with different addons has always struck me as unnecessary friction

for what it's worth, both npm and yarn have bugs (yarn@v1 specifically -- which we should stop recommending / providing an option for people to use). There are cases where I cannot get stuff working (in monorepos) with yarn@v1 or npm, because the resolution is just wrong.

It makes a lot of sense for v2 addons to use pnpm, as the monorepo support is top-notch (in addition to resolving dependencies correctly).

why should ember-cli have an an opinion about which package manager I'm using at all?

this could be philosophical and idk what the cli-core team thinks here, but to me, I would like the default experience for new apps and addons to be as bug-free as possible. That bug-free as possible store does not include npm and yarn, ime. I think supporting them is still good, as opt-in things like we have with --yarn atm.
But I think at least, that --yarn should be yarn@v3 and npm should be at least npm@v8. But because pnpm covers more of the community's use-cases (imo, ime, etc), I think pnpm should be default.

though, there is a strong argument for leaving npm as the default as "a" version of it comes with node -- so it's one less thing to install to get going

@webark
Copy link

webark commented May 18, 2022

though, there is a strong argument for leaving npm as the default as "a" version of it comes with node -- so it's one less thing to install to get going

As corepack (which I've started to use and love it at our company that uses both pnpm and yarn) becomes more mature, this is less and less of a reality as the inclusion, switching between versions, and managers is very seamless.

@bertdeblock
Copy link
Member

bertdeblock commented May 19, 2022

Probably an unpopular idea, but what if ember-cli didn't have a default package manager and left installing anything up to the user? This also makes me think of #453.

That still leaves package manager refs in README / CI / etc files though, but just wanted to mention this as an alternative.

@NullVoxPopuli
Copy link
Sponsor Contributor Author

but what if ember-cli didn't have a default package manager

Aside from setting up C.I. to work out of the box, I think it's kinda required for generating scripts, because npm, yarn, pnpm, etc all have some nuance to task/script execution (though, they are unifying on <manager> <script, then fallback to bin>

Maybe this is the best option though? idk.
on a system that already has ember projects, the install step on pnpm for newly generated projects is near instant -- and that's a really good experience

@mcfiredrill
Copy link

I don't have much to say about the details but a 3x speed improvement is going to improve developer life significantly, this sounds great. 💯

@sandstrom
Copy link
Contributor

Even though yarn and pnpm may be quicker/better, as long as the majority of the JS ecosystem is using npm, I think ember should stick to it as the default.

Basically everyone knows what npm is and how it works. That's not true for the others.

@NullVoxPopuli
Copy link
Sponsor Contributor Author

That is some good reasoning 💯

Thanks for the discussion, ya'll!

@mcfiredrill
Copy link

Even though yarn and pnpm may be quicker/better, as long as the majority of the JS ecosystem is using npm, I think ember should stick to it as the default.

I don't really agree with this. I think ember should strive to provide the best tools available as default.
If ember used this reasoning of "the rest of the JS ecosystem is doing it this way, so we should too" for every design decision, I'm not sure we would end up with the ember I know and enjoy.

@NullVoxPopuli
Copy link
Sponsor Contributor Author

that's true. The status quo is very bad

@NullVoxPopuli NullVoxPopuli reopened this Jun 25, 2022
@NullVoxPopuli
Copy link
Sponsor Contributor Author

so now I'm proposing

  • pnpm by default
  • compat with everything else

@webark
Copy link

webark commented Jun 25, 2022

If your not going to do raw npm, why not go for modern yarn? Isn't Yehuda on the yarn team anyway? And with the tighter controlled ecosystem (which will start to change a little as embroider rolls out and more raw webpack dependencies come into play), why not shoot for strict PnP support?

@NullVoxPopuli
Copy link
Sponsor Contributor Author

why not yarn?

for me, a couple reasons:

  • I've lost faith in the project
  • getting modern yarn isn't even easy. It's not even called yarn, but "yarn berry", which is more complicated to get going.

Isn't Yehuda on the yarn team anyway?

🤷

why not shoot for strict PnP support?

iirc pnpm is still more efficient (speed and disk space) -- it's all hard links -- similar to PnP tho

@webark
Copy link

webark commented Jun 25, 2022

We use pnpm pretty exclusively at my company, both for javascript/typescript apps, cdk apps, and just general mono repos orchestrators with java and python apps, and it isn't without its own misgivings.

Lerna has been revitalized as well, and it being taken over by Nrwl and its integration with Nx could bring some cool patters with how ember embraces codemods as well.

@NullVoxPopuli
Copy link
Sponsor Contributor Author

and it isn't without its own misgivings.

what have you run in to?

taken over by Nrwl and its integration with Nx

I strongly despise nx 🙃
for monorepo tooling, I strongly believe turborepo is superior, as I subscribe to the philosophy that individual packages should be in control of how they, themselves, lint, test, etc. and turborepo focuses on being a script runner, and non-invasive cacher. nx's configs leak everywhere across the monorepo and it's just not a fun or clean experience.

@webark
Copy link

webark commented Jun 26, 2022

There's been a few over the years. I ask next week to get some of them.

The main ones that I have dealt with or been a part of are it's multiple registry support, it's runtime lock analysis, and it's layered dependency resolution (though this last one seems to have been resolved in all the specific cases I have been aware with after upgrading from v5 to v6), as well as just not being as widely used (which isn't really a knock, but is sometimes barrier upon introducing some new hires).

And sure. We all have our preferences. But one of the previous points has been that the yarn/lerna combo was annoying and no longer being supported, but now that's not the case regardless your feeling upon the particulars.

@NullVoxPopuli
Copy link
Sponsor Contributor Author

v5 to v6

I've only used pnpm v7+, but it's def good to log problems with what specific versions. someone somewhere with the same problems will find this and then have more of a path to follow to finding their solution 🎉

@mcfiredrill
Copy link

I'd be open to hearing arguments for yarn 3, however

getting modern yarn isn't even easy. It's not even called yarn, but "yarn berry", which is more complicated to get going.

This has been my experience so far. I wasn't able to get yarn 3 working in an ember project, but I've got a pnpm setup pretty much working in the space of an evening.
datafruits/datafruits#909

@webark
Copy link

webark commented Jun 26, 2022

I wasn't able to get yarn 3 working in an ember project

Really? I had the opposite experience, but my attempt with pnpm was years ago, and my yarn 3 was about 6 months ago.

@NullVoxPopuli
Copy link
Sponsor Contributor Author

NullVoxPopuli commented Jun 26, 2022

Setting up pnpm is very easy in fresh ember projects

ember new my-app --skip-npm
cd my-app
pnpm install
pnpm start

tada!

(and if you don't have pnpm already, npm install -g pnpm)

(with the caveat of: ember-cli/ember-cli#9933 -- not affecting anything initially, as babel/core seems to be omnipresent -- some discussion here on that tho: ember-cli/ember-cli#9934 (yarn 3 would also report this) )

@mcfiredrill
Copy link

mcfiredrill commented Jun 26, 2022

Really? I had the opposite experience, but my attempt with pnpm was years ago, and my yarn 3 was about 6 months ago.

Well I admit @NullVoxPopuli gave me a few tips to get pnpm going that helped 🙏 😅

Anecdotally, it just seemed like there were more steps and questions to move to yarn 2/3 (what is berry? zero installs ? pnp or not?)
https://yarnpkg.com/getting-started/migration#why-should-you-migrate

@mcfiredrill
Copy link

so now I'm proposing

pnpm by default
compat with everything else

Keeping yarn compat sounds good, but yes we should not rely on yarn 1 if it's not even being developed anymore....

@webark
Copy link

webark commented Jun 26, 2022

also @NullVoxPopuli

I've only used pnpm v7+

v7 only came out a couple of months ago.. That's a pretty fresh perspective if I'm being honest.

This issue is about the default which should be done, and is mainly about documentation and best practices. I would say that the choice should either be digging in deep with a single stack, and utilizing as many aspects as possible. This i would see as favoring yarn, and really striving for strict PnP zero install builds, which is quite frankly, not an easy lift, and maybe having like yarn plugins for managing somethings 🤷‍♂️. Or being agnostic as possible, in which case I'd say stick with npm in the documentation, have a section about package manager support and mono repo setups, and use pnpm in the core ember projects. Pnpm, by its philosophy, is meant to be more hands off and agnostic anyway (in my opinion) and more just a better (dare I say performant) version of npm.

@mixonic
Copy link
Sponsor Member

mixonic commented Jul 19, 2022

We're started evaluating alternatives to Yarn 1 at work, and I ran into two concerning issues around ecosystem support for pnpm which I would like to highlight:

These seem like significant ecosystem limitations.

@SergeAstapov
Copy link
Contributor

@mixonic renovate seem to have good support for pnpm out of the box

@NullVoxPopuli
Copy link
Sponsor Contributor Author

Can confirm, i've been using renovate instead of dependabot for yeeeeaaaars

@webark
Copy link

webark commented Jul 19, 2022

Yea. We use renovate with our pnpm setup.

@mixonic
Copy link
Sponsor Member

mixonic commented Jul 20, 2022

Renovate sounds fine, however I don't think we want a default in ember cli which requires we explain to users they cannot use volta or dependabot without going off the happy path. These limitations don't make pnpm bad, or at fault, or stop it from being a viable option. They do prevent it from being a good default for a project that we all (I think) want to steadily make more and more conventional in its defaults.

@sandstrom
Copy link
Contributor

I agree.

I think Ember should pick the package manager that most people are familiar with as its default. Currently, that is NPM. Yes, there are better alternatives (which one is better depend on the project/team, for some the "best" manager is Yarn, for others it could be pnpm).

But NPM, with all its flaws, is still the de-facto standard NPM package manager. To keep friction down, I think that should be the Ember default.

This doesn't preclude us from making it easy to use other package managers, or even offer it as an command line option -- ember new --package-manager=pnpm.

Also, for everyone discussing this. Keep in mind that this choice shouldn't be optimized on what you (all tech savvy people with tons of JS/front-end experience) knows is "the best choice". Instead, it should be optimized on what makes Ember easy to get started with for the average developer. For all of you, it'll be a breeze to pick another package manager.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

I'm inclined to agree that npm should remain the default with the ability to easily choose yarn or pnpm as an alternative. Right now we have the --yarn flag so I suspect that we could generalize this a bit to support pnpm as well. I think if someone wrote an RFC for this it would be likely to be accepted.

@Turbo87
Copy link
Member

Turbo87 commented Aug 3, 2022

ember-cli/ember-cli#9879 seems relevant to this discussion 😉

@wagenet
Copy link
Member

wagenet commented Aug 3, 2022

@Turbo87 it does indeed!

@elwayman02
Copy link
Contributor

Not advocating for anything here, but just wanted to drop in and note that we recently converted our major application to Yarn 3, and I can confirm it works great in an Ember app! Regardless of what default we choose, it would behoove us to make the --yarn flag utilize Yarn 3 instead of Yarn 1 moving forward.

@bertdeblock
Copy link
Member

bertdeblock commented Oct 7, 2022

I more and more believe that it might be better to remove this responsibility from ember-cli all together.
There's a non-trivial amount of code in ember-cli to support npm, Yarn v1 and pnpm when running ember install, ember init, ember new and ember addon.
We also have Yarn vs npm conditionals in various blueprint files (pnpm support is lacking here).
We also have checks for specific versions of npm (v5 and v6) and Yarn (v2) to work around issues or bugs.
We also have to take into account e.g. Yarn workspaces to determine if Yarn should be used.
Adding support for Yarn v3 will only add to that I guess (I don't think we can just swap v1 support for v3 support?)
Consequently, I still think that #453 is a good idea and that users should use their package manager of choice to install addons and run ember g addon-name if the installed addon comes with a default blueprint (which imo, should just be a documented step in the addon's docs).
I guess a lot of users will disagree on this, but I really feel that this is a better direction to take than keep maintaining an abstraction that tries to support all/most package managers.

@sandstrom
Copy link
Contributor

@bertdeblock Yes, that makes sense. The contract could simply be that Ember assumes that node_modules exist and is populated by some external tool.

@NullVoxPopuli
Copy link
Sponsor Contributor Author

For extra context, i guess? The v2 addon blueprint supports npm, yarn, and pnpm, via some small changes to how 'scripts' are defined. https://github.com/embroider-build/addon-blueprint/blob/main/src/root-package-json.js

I agree that just deferring to a person's package manager of choice is the right call for ember-cli, but for project generating blueprints, i don't think it can be avoided due to how scripts are executed.

In the above link, we arso opted out of the ejs format (for the root package.json), because it doesn't support comments -- or rather, i've never neen comments in an ejs file (there is so much not documented about blueprints, it's nuts)

@johanrd
Copy link

johanrd commented Apr 17, 2023

@NullVoxPopuli I have yet to get embroider + pnpm to be working with some (quite popular) ember-addons, see e.g. this reproducible example: kturney/ember-mapbox-gl#122 .

@NullVoxPopuli
Copy link
Sponsor Contributor Author

do you have a reproduction example/repo? I haven't seen this problem before.

@johanrd
Copy link

johanrd commented Apr 17, 2023

yes, see https://github.com/johanrd/ember-mabox-gl-pnpm-repro

or perhaps simpler: 1) create a fresh ember app with pnpm:

ember new ember-mabox-gl-pnpm-repro --skip-npm
cd ember-mabox-gl-pnpm-repro
pnpm install
pnpm add ember-mapbox-gl mapbox-gl
  1. Run pnpm start and experience the following error:
Cannot find module '.pnpm/package.json' from '/Users/me/ember-mabox-gl-pnpm-repro'

@NullVoxPopuli
Copy link
Sponsor Contributor Author

excellent, I'll debug

@NullVoxPopuli
Copy link
Sponsor Contributor Author

See this line: https://github.com/kturney/ember-mapbox-gl/blob/master/index.js#L21

There are three bugs:
0. this isn't even embroider 😅

  1. ember-mapbox-gl uses mapbox-gl v1, yet v2 is installed
  2. ember-mapbox-gl did not declare a peer on mapbox-gl, so node doesn't know how to resolve mapbox-gl
  3. ember-cli is running the included hook from within its own context, and require.resolve is starting from the wrong location (ember-cli instead of ember-mapbox-gl)

regarding peers and dependency resolution, and if it were only this problem:
it's trying to resolve mapbox-gl/package.json relative to that index.js.
but mapbox-gl is not declared as a peerDependencies in ember-mapbox-gl.
This would happen with yarn3/4 as well as other newer package managers, and isn't just a pnpm issue or any issue with a package manager at all.

Thankfully, pnpm is aware the npm ecosystem is a hot mess, and provides us ways to "keep being productive": https://pnpm.io/package_json#pnpmpackageextensions or patch-package: https://pnpm.io/cli/patch

I would patch-out this included method altogether -- app.import is on its way out, though still supported for now.

As a work around, you can call the app.import method from your ember-cli-build.js file in your app.

@elwayman02
Copy link
Contributor

elwayman02 commented Apr 17, 2023

I think a better goal for this RFC would be to get pnpm, yarn 3, and latest npm all well-supported out of the box with the appropriate flags, while maintaining the current default (npm). Achieving that state would be a reasonable stepping stone to evaluate whether the default should ever be moved away from npm, because it would allow projects to easily explore switching to pnpm (or starting new projects with it) and identify ecosystem issues that need to be addressed (like volta/dependabot compatibility) before it could be a reasonable default. As it is right now, there's no out-of-the-box for pnpm at all, which means it's unlikely the community as a whole is going to explore it enough to ever reasonably make the decision being asked for in this RFC.

@NullVoxPopuli
Copy link
Sponsor Contributor Author

Gonna close this issue, as I forgot about it, and agree with npm being default.

We'll need to test against pnpm and yarn3+.

Also adding support is here for pnpm is here: #907

best have support before changing defaults.

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

No branches or pull requests