-
-
Notifications
You must be signed in to change notification settings - Fork 88
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
feat: support other js package managers #349
Conversation
cb43676
to
48e80e7
Compare
@ahangarha @justin808 I would appreciate some initial feedback as I think it'll require a major change and probably a bit of overhauling of the test suite so want to make sure you're ok with the general direction before I continue to sink more time into it. I would also like to land the addition of |
I like the concept! My main concern is the balance of the:
|
I don't think so so long as Shakapacker (& co) don't try to do too much - imo the focus for gems like Shakapacker and That also goes for In practice this means the main use-case of A good example of this is with Shakapacker technically already stretches this because it provides bin scripts for I expect that approach should make it easier to be compatible across different major versions and managers, and I think that's already been validated somewhat by how straightforward the logic and test suite for
I think that lies in the eyes of the downstream developer - people will tell you that there are advantages to each of the different managers, and they're not wrong though I think it's not entirely wrong either to argue just one of those managers is enough. I wouldn't say this is necessarily something Shakapacker & co need, but I think it would make them better since it would be giving people downstream more flexibility. For what it's worth, we prefer |
886f364
to
38ce7fd
Compare
fwiw I've got CI passing for my PR to I'm going to attempt to make this opt-in to start with. |
3c2d16c
to
faa03ae
Compare
b76e99f
to
faa03ae
Compare
9ddce58
to
3fe304f
Compare
@G-Rath what's pending on this draft PR? Is this ready for final review? |
3fe304f
to
6bc5448
Compare
@justin808 yup I think it should be good to get at least an initial review - CI is green for rails-template and react-rails so I'm pretty confident this works, I just need to write some tests; I ended up feature flagging this, which is why CI is currently passing.
|
8d288ba
to
8e9db60
Compare
It seems that requires which happen after this are mucked up due to the load paths having been changed - this really only seems to impact RSpec which requires the diff library lazily
…e_json` is enabled
…e everything is valid
98b6401
to
36be318
Compare
@justin808 have added a changelog - I think we're good to get this landed and do a new release |
So nice, please lets get this merged! ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
This adds support for package managers other than
yarn
by using a new prototype library I've created calledpackage_json
which provides an abstraction over top ofpackage.json
and the respective managers.Related to #195
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
~
.Other Information
Foremost I'd like to get feedback from the maintainers about if this is an approach they'd be ok with; beyond that I'm happy to have feedback both on the implementation here & related repos, and on
package_json
itself - at this point the underlying logic is pretty stable, but I'm very much open to changing the API if the folks have opinions.I've got this working locally and in our
rails-template
with all three package managers - I'm still working on getting the test suites for here andreact-rails
passing, and will also be adding more actual tests torails-template
to confirm everything is actually working with the different managers.Note that currently Yarn Berry is not supported by
package_json
mainly because it can't be installed vianpm
like the others can - I've already looked into the mappings for its commands, and will tackle that once I've got everything across the board.This will also require a similar change to
react-rails
which I've got locally but will do that PR later.