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

Ignore https-proxy-agent dependency in browser environments #2133

Merged
merged 3 commits into from
Nov 16, 2022

Conversation

mpint
Copy link
Contributor

@mpint mpint commented Nov 10, 2022

High Level Overview of Change

This change ignores https-proxy-agent from browser environments in package.json, mirroring the existing content in https://github.com/XRPLF/xrpl.js/blob/main/packages/xrpl/webpack.config.js#L59

Context of Change

With some effort in our Vite.js based project, we were able to polyfill node dependencies to resolve this known issue.

However, this fix did not extend to our Cypress test environment. With a working Vite configuration, running Cypress tests yields:

Error: Build failed with 2 errors:
node_modules/https-proxy-agent/dist/agent.js:15:38: ERROR: Could not resolve "net"
node_modules/https-proxy-agent/dist/agent.js:16:38: ERROR: Could not resolve "tls"
    at failureErrorWithLog (/node_modules/esbuild/lib/main.js:1566:15)

To get Cypress tests running, we:

  1. Utilized @bahmutov/cypress-esbuild-preprocessor to polyfill esbuild node dependencies to match those in our Vite configuration (including aliases)
  2. Added "https-proxy-agent": false to xrpl.js package.json's browser field.

This has the effect of ignoring http-proxy-agent in browser environments, which clears up the error and allows Cypress tests to run. See the spec for more information.

Linting changes

This change (adding a boolean value to package.json) triggers this bug in eslint-plugin-import, which motivated the temporary disablement of the import/no-unused-modules linting rule. After the eslint-plugin-import bug is fixed, import/no-unused-modules can be enabled again (e.g. revert this commit and turn back on import/no-unused-modules

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Before / After

https-proxy-agent is no longer included in browser environments.

@mvadari
Copy link
Collaborator

mvadari commented Nov 14, 2022

Looks like the linter is failing.

@mpint mpint force-pushed the ignore-https-agent-browser branch 5 times, most recently from 646686b to a8c3dfa Compare November 15, 2022 19:58
Copy link
Contributor

@JST5000 JST5000 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@mvadari mvadari left a comment

Choose a reason for hiding this comment

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

Please update the changelog

@mvadari mvadari merged commit a4c2bb9 into XRPLF:main Nov 16, 2022
@mpint mpint deleted the ignore-https-agent-browser branch November 16, 2022 23:01
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.

2.0@beta error TypeError: Right-hand side of 'instanceof' is not an object
4 participants