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(skymp5-client): revert manual host resolution #2321

Merged
merged 2 commits into from
Feb 8, 2025

Conversation

nic11
Copy link
Collaborator

@nic11 nic11 commented Feb 8, 2025

Important

Removes manual DNS resolution in skympClient.ts, using server-ip directly for connections.

  • Behavior:
    • Removes manual DNS resolution logic in skympClient.ts.
    • Directly uses server-ip from settings for connection.
  • Functions:
    • Deletes resolveHost() function in SkympClient class.
    • Updates establishConnectionConditional() to use targetIp directly.

This description was created by Ellipsis for 81b6db1. It will automatically update as commits are pushed.

@Pospelove Pospelove marked this pull request as ready for review February 8, 2025 16:26
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 81b6db1 in 33 seconds

More details
  • Looked at 42 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. skymp5-client/src/services/services/skympClient.ts:1
  • Draft comment:
    Removal of DNS resolution: The manual host resolution function and its dependencies (resolve4, promisify) were removed, which is acceptable given the config now uses an IP directly.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. skymp5-client/src/services/services/skympClient.ts:93
  • Draft comment:
    Direct usage of targetIp from settings is clear and concise. Ensure that the configuration 'server-ip' is validated externally as this change bypasses DNS resolution.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. skymp5-client/src/services/services/skympClient.ts:14
  • Draft comment:
    Removed manual DNS resolution via 'resolve4' and 'promisify'. Ensure the 'server-ip' setting always provides a valid IP.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. skymp5-client/src/services/services/skympClient.ts:20
  • Draft comment:
    Changed config key from 'server-host' to 'server-ip'. Confirm that the configuration and docs reflect this change.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. skymp5-client/src/services/services/skympClient.ts:89
  • Draft comment:
    The 'establishConnectionConditional' method is marked async but no awaits are used now. Consider removing async or adding error handling if needed.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_VlQylLRR7pHcVYcN


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@Pospelove Pospelove merged commit c457673 into skyrim-multiplayer:main Feb 8, 2025
14 checks passed
@Pospelove Pospelove deleted the revert-resolve branch February 8, 2025 16:29
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 participants