feat!: always use package_json for managing node packages

[G-Rath]

Summary

This switches us over to using package_json for managing node dependencies, which was previously an opt-in feature - after this, developers will be able to use any of the five major package managers with their Rails applications, with the default being npm

Pull Request checklist

Remove this line after checking all the items here. If the item does not apply to the PR, both check it out and wrap it by ~.

• Add/update test to cover these changes
• Update documentation
• Update CHANGELOG file

Other Information

I still have to update the documentation - generally I'm leaning towards sticking a section near the top about how you can use any major you like and the default is npm, and then switch the rest of the docs over to using npm in examples instead of yarn.

[tomdracz]

Curious on npm default rationale - how did we land here? Traditionally yarn was the only one supported so this feels like a fairly inconvenient change for upgrade potentially. Is it just the nature of package-json gem?

[G-Rath]

Because that aligns with the default for Node / package.json - npm is the default package manager that ships with Node and so in the absence of a packageManager in package.json, is what gets used; that's also why npm is not actually listed as a supported package manager with corepack.

Really the biggest impact of this should be when installing packages, which as of v8 we won't do automatically - running commands with a different package manager is usually fine (I use my nrun script locally for running commands which uses always npm under the hood, and not had any issues), so this should primarily come up when you're doing shakapacker:install which is typically at the start of a fresh project so should just mean having to spend 5 minutes figuring out how to switch back to Yarn if that's what you want.

I do have an idea on a way we might be able to handle this a bit more gracefully but it would add a bunch of extra faff and I'm keen to get us being package manager agnostic by default so I'd rather go ahead with this and then later follow-up with the "graceful improvement" in a minor version if we do get feedback that highlights it worth the faff.

I will make it clearer here though that people should ensure packageManager is set as part of the upgrade

[justin808]

I'm still not a fan of switching away from yarn as the default unless there's a good reason.

https://chat.openai.com/share/d33fe865-defd-41df-ac89-4db307a71052

[G-Rath]

That AI conversation is not really useful because it doesn't have the full context - ultimately shakapacker itself shouldn't be setting a default as it's a library, and so npm is the default because that's the default in Node which is the default runtime shakapacker targets.

This change means effectively shakapacker aligns with node in terms of switching package managers so if you want to use yarn you just follow the recommended steps (i.e. corepack and packageManager) and both Node and shakapacker will respect that and just work 🙂

The only place we're selecting npm as an opinion is in the docs but that's because I don't think it's worth the overhead of documenting every possible combination of every package manager 🤷‍♂️

[tomdracz]

Thanks for the doc updates, LGTM but let's get more eyes on this and can merge if everyone else is happy

[ahangarha]

The next tag on npmjs refers to v6.0.0-rc.14.
I'm not sure why it is so, but that is not what we want to use here.

Though not related to this PR, it is worth it to review it again.

CC: @justin808

[justin808]

@G-Rath @tomdracz why is the default switching from yarn to npm? That seems like a big change.

[G-Rath]

@justin808 see this comment

[tomdracz]

Talked briefly with @justin808 and in two minds about this default. I hear the point around matching node default (since you will always get at least NPM), but think Rails has mostly set on using yarn and requires bit of extra work to get other package managers working.

How hard would it be to retain yarn default? Is adding pkg manager into package.json something we could automate? Some existing tool detection akin to https://github.com/rails/jsbundling-rails/blob/main/lib/tasks/jsbundling/build.rake?

As said elsewhere, switching manager after install shouldn't be a rocked science so not opposed to new default in principle, but given the ecosystem, I wonder if we can make upgrade less annoying (those 5 mins switching back to yarn and having package-lock.json when I've had yarn before WILL annoy me 😅) or find some other compromise?

[tomdracz]

If we don't find a good compromise path though, I would say that IMO benefits of choice of pkg manager outweigh the pain of switching

[G-Rath]

Rails has mostly set on using yarn and requires bit of extra work to get other package managers working.

From what I've seen that was covered by Webpacker which is now Shakapacker, and so because that now supports multiple package managers there is no "bit of extra work" required (beyond setting up the package manager of choice, which isn't our cost to pay) - it's literally set the packageManager property and it just works (I've already done this for a few apps at Ackama which are using NPM in production).

The thing that makes it tricky is packageManager requires an exact version and Yarn has two version different versions in v1 (classic) and v2+ (berry) so ironically it's the hardest package manager to guess for.

The reason why jsbundling can get away with such a simple check is because it purposely tries to interact with Node as little as possible whereas Shakapacker has a much smarter integration that requires it to understand more about the package manager its using e.g. to pass in commands.

I wonder if we can make upgrade less annoying

This is imo the hardest thing to address because unlike install users could just not follow the upgrade path and run into this all over the place so we'd have to stick a check or guard around pretty much every method and have that be there for the whole of v8.

It might be ok though 🤔

This has also inspired me to have another look at the install logic, as I think we can probably do better about setting packageJson.

[G-Rath]

@tomdracz fwiw your self-review request dismissed your previous approval and I can't re-request since it's technically already been re-requested 😅

[G-Rath]

@tomdracz @justin808 could you have a look over the new ManagerChecker class I've pushed up and let me know if it is going in a direction you're happy with?

I'll get the whole change finished off later this week but at the high-level what I'm thinking is:

• Shakapacker uses npm by default but will check that against existing lockfiles
  • so for completely fresh apps that rely on Shakapacker to initially create the package.json, the default will be npm (or whatever PACKAGE_JSON_FALLBACK_MANAGER is set to)
• shakapacker:install merges with existing package.json instead of replacing it, and tries to set packageManager if not present
  • this way users can initialize their codebase / package.json with their manager of choice (which'll generate a lockfile we can guess the manager from)
  • so far I'm not planning to add tests to cover the merge flow because it would require doubling the generator specs which are already slow and plentify, and I'm not sure if it'll be worth it; let me know if you're happy with that
• Shakapacker will call Shakapacker::ManagerChecker.new.warn_unless_package_manager_is_obvious! when it starts (or before a command..?)
  • this prints a deprecation message with the right thing to add to package.json; while it might seem weird that we know what should happen but don't do it, I think that could have too many edge cases
  • alternatively, we could exit after printing the warning rather than continue?

I think the case could be made that some of this logic should go into package_json (and indeed some it was brought from there) but currently it's not clear enough to me how that should look with the current API so I think we should just ignore that for now - once we've lived with this for a while, we can review if it makes sense to move around.

[G-Rath]

@tomdracz I think this should be ready for review - I'm pretty sure the dummy specs failure is just a fluke; I've still got to update the changelog but the rest of it should be ok to get reviewed.

[tomdracz]

One change to fix failing dummy specs. Otherwise looks sensible. Could definitely use more eyes though @justin808 @ahangarha @Judahmeek

[tomdracz]

We need the same in spec/dummy/package.json. Lack of this is causing failures there. Wonder if this lacks some better handling.

To repro dummy fail:

- remove `spec/dummy/public/packs-test` directory
- run bundle exec rake run_spec:dummy

This should fail. Trying to run rake assets:precompile in the dummy dir:

rake aborted!
Shakapacker::Utils::Manager::Error: You have not got "packageManager" set in your package.json meaning that Shakapacker will use npm
but you've got a yarn.lock file meaning you probably want
to be using yarn instead.

To make this happen, set "packageManager" in your package.json to [email protected]
/Users/tom.dracz/projects/shakapacker/lib/shakapacker/utils/manager.rb:25:in `error_unless_package_manager_is_obvious!'
/Users/tom.dracz/projects/shakapacker/lib/tasks/shakapacker/check_manager.rake:7:in `block (2 levels) in <main>'

So the compile on the fly probably also fails but the issue is non-obvious. Should check how it behaves in real-life scenario to see if that's anything to patch 🤔

[G-Rath]

Interesting, it looks like this is a general bug somewhere though I can't quite tell if its in shakapacker or react_on_rails - any error that gets thrown results in this, assumably because something is eating that error.

I've confirmed this occurs in v7 but I'll see if I can pin down what's happening (it's tough though cause seriously everything is being eaten - so no binding.pry, no puts output, no raise, etc)

[G-Rath]

it does look like the version mismatch warning/error gets outputted correctly, which I notice is called in railtie.rb - I'm not super familiar with how railties actually work but should we actually be doing this check there instead?

[G-Rath]

oh hey yeah test output is going to spec/dummy/test.log, which has all the stuff - maybe the bug is that something isn't checking an exit code? fwiw I've tested this on some of my apps which use react-rails and they error fine.

[G-Rath]

interestingly setting config.action_dispatch.show_exceptions = :rescuable (which is the value we use at Ackama but currently false in spec/dummy) in config/environments/test.rb results in the tests passing - is it possible we're hitting an error that isn't actually an error..?

either way @tomdracz I've pushed up switching to using railtie to do the check - let me know if you think that's the right move, and we can pick up from there

[tomdracz]

Thanks, I'll play around. With this particular thing I presume it just might be React on Rails swallowing error and raising another one

[tomdracz]

Looks like at least we're erroring out early so nice improvement. 3a566ef to patch the dummy spec failure itself

[tomdracz]

In two minds about this:

• if we guess binary, could we use guessed one and warn rather than error?
• On the other hand, at some point in the future we will probably want to raise so maybe good to have this screaming at the get go in the major version

[G-Rath]

I think erroring is the right way to go for now - the problem is this is all about predicting what package_json needs and that could be used by other gems; so if we were to try using the guessed version here it would only be fixing the problem for us, and this is also a guess so we could be wrong...

Ideally the guessing logic should move to package_json but I don't want to do that yet because there might be some edge cases we've missed, and it would be a non-breaking feature to land.

Also keep in mind this is a one-off change to resolve that will only happen if you've not followed the upgrade guide.