fix: throw error on initialize fail

[francescoopiccoli]

Problem

Follow up to the discussion in this merged PR: #306

In this PR: we revert accessing aws object safely, and return a ResponseError if initialization fails.

The previous PR (#306) was mistakenly trying to both mitigate and log the issue, when only one of the two objectives is possible since they are contrasting.
As stated in this comment (#307 (comment)) we will proceed with only logging since it will help in finding the root cause with the issue of aws object being undefined.

Solution

• Remove optional chaining when accessing aws
• Make sure to throw error even after catching it for logging purpose, this way make sure that LSP server won't start in case of error in initialization.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[justinmk3]

#306 was hard to review partly because of the lack of a test. Is there a test for init scenarios, or why not?

"perf" implies this is a performance related improvement. But this is more like a bug fix.

[justinmk3]

This PR should also improve the try-catch as mentioned in #306 (comment)

[francescoopiccoli]

#306 was hard to review partly because of the lack of a test. Is there a test for init scenarios, or why not?

"perf" implies this is a performance related improvement. But this is more like a bug fix.

@justinmk3 Thanks for reviewing. I changed the "perf" to "fix" as suggested.
You are right about adding tests, it would be beneficial, we will address it in a follow up PR to merge the fix in the meanwhile.

[francescoopiccoli]

After discussion with @volodkevych and @imykhai, we realized that either we mitigate or we log the failures to help toolkit find the root cause of the issue. Hence, we will revert the current changes to make sure to log any failure to aws being undefined.

[awschristou]

Please decorate the returned error message with

initialization options: ${JSON.stringify(params.initializationOptions)}`

This may seem repetitive, but what happens in line 108 and line 111 end up surfacing in different locations in the client, and users are more likely to give us the toolkit log, where this message is written. More importantly, we will receive this (line 111) string in our metrics, and this is the change I was initially asking for.

[awschristou]

Before proceeding, I'd like to see if we can get the initialize payload sent as part of the initialize response.

[justinmk3]

The try still wraps too much code. I guess that can be a followup, like the test coverage.