Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

parse package.json instead of running npm|yarn list #1690

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

cecyc
Copy link
Contributor

@cecyc cecyc commented Oct 26, 2021

WHY are these changes introduced?

Fixes #1683

WHAT is this pull request doing?

Instead of running npm | yarn list to find the renderer_package version that is needed for the extension to run, parse the package.json file and get the version from there.

Pros:

  • This reduces code complexity as we no longer have to make a JS call to find the renderer version
  • Reduces issues when there are multiple versions of a dependency when running npm | yarn list
  • There is a known bug with the list command on earlier versions of Node, and not having to run the command would eliminate said bug

How to test your changes?

  • Run shopify extension serve, shopify extension register and shopify extension push on a PRODUCT_SUBSCRIPTION extension

Tophatted:

Product Subscription

  • javascript
    • serve
    • register
    • push
  • typescript
    • serve
    • register
    • push
  • javascript react
    • serve
    • register
    • push
  • typescript react
    • serve
    • register
    • push

Checkout UI

  • javascript
    • serve
    • register
    • push
  • typescript
    • serve
    • register
    • push
  • javascript react
    • serve
    • register
    • push
  • typescript react
    • serve
    • register
    • push

Post Purchase

  • javascript
    • serve
    • register
    • push
  • typescript
    • serve
    • register
    • push
  • javascript react
    • serve
    • register
    • push
  • typescript react
    • serve
    • register
    • push

Post-release steps

N/A

Update checklist

  • I've added a CHANGELOG entry for this PR (if the change is public-facing)
  • I've considered possible cross-platform impacts (Mac, Linux, Windows).
  • I've left the version number as is (we'll handle incrementing this when releasing).
  • I've included any post-release steps in the section above.

@cecyc cecyc requested review from a team, pepicrft and hannachen and removed request for a team October 26, 2021 22:16
Copy link
Contributor

@t6d t6d left a comment

Choose a reason for hiding this comment

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

Changes look mostly good to me. I left a suggestion, one follow up for @vividviolet and a question regarding some stubs in the test suite.

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Thanks for improving this @cecyc. It'll make the package detection in projects much more reliable 🚀

@cecyc
Copy link
Contributor Author

cecyc commented Oct 29, 2021

@pepibumur Implemented most of your suggestions, thank you so much for the feedback!

The only things I could not implement were:

  • Adding support for --path. I like this idea and I want to do this in the future, but it's not something I want to implement without speccing it out more, plus I want this fix to go out on Monday :)
  • Not stubbing out standard libraries: I generally agree with this as well, and I think adding support for path will help in implementing your suggestion in the future

LMK if you have any other thoughts, like I said, my goal is to merge this Friday Oct. 29 so it can go out with Monday's release on Nov. 1.

Thanks!

@cecyc cecyc requested review from t6d and pepicrft October 29, 2021 04:26
@cecyc cecyc mentioned this pull request Oct 29, 2021
4 tasks
@pepicrft
Copy link
Contributor

That's awesome, thanks @cecyc. I agree, we can go ahead and merge and keep this in mind going forward for future PRs. Excellent job 👏🏼

Copy link

@elanalynn elanalynn left a comment

Choose a reason for hiding this comment

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

🎩'd with react.tsx template and all the commands ran successfully. Nice work!

@cecyc cecyc merged commit c370cae into main Oct 29, 2021
@cecyc cecyc deleted the feature/parse-package-json branch October 29, 2021 20:02
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems November 1, 2021 21:38 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting Extension template references invalid renderer package when installing/serving a React extension
5 participants