Sm/w 8206747 #347
Merged
Sm/w 8206747 #347
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mshanemc
requested review from
amphro,
WillieRuemmele,
shetzel,
peternhale and
rorolopetegui
mshanemc
commented
Dec 22, 2020
mshanemc
commented
Dec 22, 2020
mshanemc
commented
Dec 22, 2020
mshanemc
commented
Dec 22, 2020
mshanemc
commented
Dec 22, 2020
peternhale
requested changes
Dec 28, 2020
src/connection.ts
Outdated
Comment on lines
126
to
130
| try { | ||
| await conn.useLatestApiVersion(); | ||
| } catch (e) { | ||
| conn.logger.error(`Cannot connect to the org at ${conn.options?.connectionOptions?.instanceUrl}`); | ||
| } |
There was a problem hiding this comment.
useLatestApiVersion also has a try/catch and it does not rethrow, so this try/catch seems unnecessary.
There was a problem hiding this comment.
Did some updates after Steve's feedback and a lot more e2e testing. That try/catch in useLatestApiVersion is now more error-specific for DNS problems.
There was a problem hiding this comment.
Not sure what other scenario would prevent us from being able to determine the version, though.
|
Looking better already!
|
peternhale
reviewed
Dec 30, 2020
peternhale
reviewed
Dec 30, 2020
@mshanemc, while the new error message "Org Not Found" is a great clarification, it does change the existing output. Should Claire/Juliet be consulted on this change to the output? |
peternhale
reviewed
Dec 30, 2020
peternhale
reviewed
Dec 30, 2020
peternhale
requested changes
Jan 4, 2021
peternhale
approved these changes
Jan 5, 2021
shetzel
approved these changes
Jan 5, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@W-8206747@
I hate everything about this fix. From what I can tell, the stuff leaking into the stdout isn't from us, it's from jsforce > request > asap
Asap hasn't been updated in years and request is deprecated. This'll happen whenever jsforce.request tries to use a domain that no longer exists and gets a DNS ENOTFOUND.
You can most easily repo by creating a sandbox, authing to it, and then deleting it, and then waiting for a while for it to be destroyed.
After a few tries, the best option I could think of is to pre-check a DNS using our existing domainResolver. This required a number of updates to unit tests (where instanceUrl must exist to get past the pre-check). It was either that or a serious rewrite the tests in a major way.