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

Fix: Tunnel --PORT parameter not working in Node.js app. #1763

Merged
merged 6 commits into from
Nov 18, 2021

Conversation

paul-phan
Copy link
Contributor

@paul-phan paul-phan commented Nov 17, 2021

WHY are these changes introduced?

After version 2.7.0, we no longer can use a custom port for ngrok tunnel. Seem like there was a missing parameter in the node_service.rb

WHAT is this pull request doing?

Add the port parameter to the tunnel start function.

How to test your changes?

For Node.js project, assume you want to tunnel the localhost:3000.
This command should work: shopify app serve --PORT 3000

Post-release steps

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.

@paul-phan paul-phan requested review from a team, hannachen and gonzaloriestra and removed request for a team November 17, 2021 03:31
@ghost ghost added the cla-needed label Nov 17, 2021
Copy link
Contributor

@gonzaloriestra gonzaloriestra 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 your contribution, @paul-phan! 👏

Your changes look right. However, we would need a few things:

  • Could you sign the Contributor License Agreement (CLA)?
  • Add this change to the CHANGELOG.md (Unreleased section)
  • Would you mind adding tests for this? I think it would be enough if you add something like ShopifyCLI::Tunnel.expects(:start).with(@context, port: 5000).returns("https://example.com") to the tests named test_call_when_port_passed and test_server_command_when_port_passed

lib/shopify_cli/version.rb Outdated Show resolved Hide resolved
@ghost ghost removed the cla-needed label Nov 18, 2021
Copy link
Contributor

@gonzaloriestra gonzaloriestra 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 the changes! I've already tested it and it works as expected 👌

There are just a few minor details to be fixed and we can merge it!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
test/shopify-cli/services/app/serve/rails_service_test.rb Outdated Show resolved Hide resolved
test/shopify-cli/services/app/serve/node_service_test.rb Outdated Show resolved Hide resolved
@paul-phan
Copy link
Contributor Author

Thanks for your suggestion; I've updated it!

Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

Ready to merge 🚀

It will be included in the next release. Thanks!

@gonzaloriestra gonzaloriestra merged commit beeaab2 into Shopify:main Nov 18, 2021
@jesalerno84 jesalerno84 mentioned this pull request Nov 30, 2021
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems November 30, 2021 18:12 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.

2 participants