Skip to content
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(create-remix): Add proxy support in create cli #4159

Merged
merged 8 commits into from
Oct 12, 2022

Conversation

freeman
Copy link
Contributor

@freeman freeman commented Sep 7, 2022

Closes: #3159

  • Docs
  • Tests

Testing Strategy:

I tested the built cli behind a proxy and it worked seamlessly.

@changeset-bot
Copy link

changeset-bot bot commented Sep 7, 2022

🦋 Changeset detected

Latest commit: f4f1dc1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
remix Patch
@remix-run/dev Patch
create-remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/vercel Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Sep 7, 2022

Hi @freeman,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@freeman freeman changed the title Proxy support cli Proxy support in create cli Sep 7, 2022
@freeman freeman force-pushed the proxy-support-cli branch 2 times, most recently from c78e48f to 1b3b28d Compare September 7, 2022 18:26
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Sep 7, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@freeman
Copy link
Contributor Author

freeman commented Sep 10, 2022

slightly simplified the code by having a single agent for the whole file

@freeman
Copy link
Contributor Author

freeman commented Sep 15, 2022

Let me know if I should do something to get this merged.

This is adding friction to get remix adopted at work :)

@machour machour requested a review from mcansh September 20, 2022 13:51
@machour machour changed the title Proxy support in create cli feat(create-remix): Add proxy support in create cli Sep 20, 2022
@mcansh
Copy link
Collaborator

mcansh commented Sep 20, 2022

thanks for taking this on @freeman! i've had this on my backlog for far too long 😅. looks good to me on first glance

@freeman
Copy link
Contributor Author

freeman commented Sep 20, 2022

Working out the test failures

@freeman
Copy link
Contributor Author

freeman commented Sep 20, 2022

Issues appears only in test with msw because agent-base has a pretty fragile logic for determining the agent protocol

https://github.com/TooTallNate/node-agent-base/blob/c011b2b8efc8de6d57600c459d49baa4c608b86b/src/index.ts#L15-L19

This logic breaks under msw because it overrides the normal node request processing (for good reasons !).

I saw 2 paths to fix this:

  • add some logic to ensure the agent protocol is set correctly based on the target url (even if in theory this is not needed outside of the test harness)
  • find a way to bypass using the agent at all if under test harness

I went with the first option as some people noted that the logic of relying on stack trace could be broken in future node versions (TooTallNate/node-agent-base#63).

@freeman
Copy link
Contributor Author

freeman commented Sep 23, 2022

@mcansh could you approve the workflow run if you're ok with the changes to allow tests to pass ?

@mcansh
Copy link
Collaborator

mcansh commented Sep 29, 2022

@freeman thanks again! do you know if it would be possible to add a test for this new behavior? I haven't worked much behind a proxy 😅

@freeman
Copy link
Contributor Author

freeman commented Sep 29, 2022

@mcansh I'll take a look but this seems complex to test. I'd need to spin up a temporary proxy server (this seems doable, there is a proxy package) and try to ensure it has been used without breaking the encapsulation...

@freeman
Copy link
Contributor Author

freeman commented Oct 11, 2022

@mcansh added a basic test that verifies that proxy env variable is honored.

@mcansh mcansh merged commit 6d1a317 into remix-run:dev Oct 12, 2022
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-eebaa56-20221013 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants