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

fix(request): proxy-agent breaks Studio #580

Merged
merged 5 commits into from
Sep 24, 2019
Merged

fix(request): proxy-agent breaks Studio #580

merged 5 commits into from
Sep 24, 2019

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Sep 20, 2019

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

This is almost impossible to cover with tests, as Studio has quite a specific setup. We just gotta be careful and avoid running Node specific code as much as possible.

@P0lip P0lip added the t/bug Something isn't working label Sep 20, 2019
@P0lip P0lip self-assigned this Sep 20, 2019
src/request.ts Outdated
const options =
typeof process === 'object' && process.env.PROXY ? { ...opts, agent: new ProxyAgent(process.env.PROXY) } : opts;
if (typeof process === 'object' && process.env.PROXY) {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to import every time a request is made? Is that gonna be ok for performance/memory usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!
Luckily, Node.js caches modules (can be checked under require.cache), therefore they are loaded and executed only once unless one clears the cache.
That said, we should be okay.

Note, even though import is used here, it's replaced with a require call when we compile the code from TS to JS.

I don't take bundlers into account, since we don't aim to run this code in a browser, but they are no different.

@P0lip P0lip requested a review from philsturgeon September 23, 2019 19:21
====stdout====

{document}
2:9 error invalid-ref FetchError: request to http://localhost:3002/foo.json failed, reason: connect ECONNREFUSED 127.0.0.1:3001
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a tad dumb. We could spin up a server.
Should be fine for now.

@P0lip P0lip merged commit bfb15f8 into develop Sep 24, 2019
@P0lip P0lip deleted the fix/http-agent branch September 24, 2019 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants