Skip to content

refactor: create a dedicated method for updating package.json#527

Merged
G-Rath merged 1 commit intomainfrom
update-package-json-better
Jan 26, 2024
Merged

refactor: create a dedicated method for updating package.json#527
G-Rath merged 1 commit intomainfrom
update-package-json-better

Conversation

@G-Rath
Copy link
Copy Markdown
Contributor

@G-Rath G-Rath commented Jan 25, 2024

I'm aware of how much conflict this will create with #466 but oh well 🤷

@G-Rath G-Rath force-pushed the update-package-json-better branch 2 times, most recently from 08958b0 to 008e79d Compare January 25, 2024 19:52
Copy link
Copy Markdown
Contributor

@joshmcarthur joshmcarthur left a comment

Choose a reason for hiding this comment

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

I'm fine with this in principle, but we use template files a lot for similar use cases. I just wanted to prompt whether a template would be cleaner than doing a lot of file substitutions from code like this

@G-Rath
Copy link
Copy Markdown
Contributor Author

G-Rath commented Jan 25, 2024

We can't use templating because the file is touched by external code too i.e. react-rails and shakapacker write to it (and in fact we're depending on shakapacker:install to create the file initially).

Also I don't think it's as comparable: afaik all our templating files are generally within a single variant or maybe two whereas this particular file is modified by 5, and which are somewhat dependent on the actions of each other so I don't think templating would be as clean of a solution here.

@joshmcarthur
Copy link
Copy Markdown
Contributor

Cool, that's all good. I thought it was worth raising, because it's quite easy to take an established pattern (like modifying the file in code), and use that over something that's already available via thor and the generator framework

@G-Rath G-Rath marked this pull request as ready for review January 25, 2024 22:41
@G-Rath G-Rath force-pushed the update-package-json-better branch from 008e79d to dd79577 Compare January 25, 2024 23:33
@G-Rath G-Rath merged commit c6801a5 into main Jan 26, 2024
@G-Rath G-Rath deleted the update-package-json-better branch January 26, 2024 00:29
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

Successfully merging this pull request may close these issues.

3 participants