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(plugin): decouple state update from the LS #2643

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

dankeboy36
Copy link
Contributor

@dankeboy36 dankeboy36 commented Feb 25, 2025

Motivation

To enhance the reliability of Arduino IDE extensions, the update process for ArduinoState has been modified to ensure independence from the language server's availability. This change addresses issues caused by compileSummary being undefined due to potential startup failures of the Arduino Language Server, as noted in dankeboy36/esp-exception-decoder#28 (comment).

Change description

The compile command now resolves with a CompileSummary rather than void, facilitating a more reliable way for extensions to access necessary data. Furthermore, the command has been adjusted to allow resolution with undefined when the compiled data is partial.

Other information

By transitioning to direct usage of the resolved compile value for state updates, the reliance on executed commands for extensions is eliminated. This update also moves the VSIX command execution to the frontend without altering existing IDE behavior.

Closes #2642

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

To enhance the reliability of Arduino IDE extensions, the update
process for `ArduinoState` has been modified to ensure independence
from the language server's availability. This change addresses issues
caused by `compileSummary` being `undefined` due to potential startup
failures of the Arduino Language Server, as noted in
dankeboy36/esp-exception-decoder#28 (comment).

The `compile` command now resolves with a `CompileSummary` rather than
`void`, facilitating a more reliable way for extensions to access
necessary data. Furthermore, the command has been adjusted to allow
resolution with `undefined` when the compiled data is partial.

By transitioning to direct usage of the resolved compile value for
state updates, the reliance on executed commands for extensions is
eliminated. This update also moves the VSIX command execution to the
frontend without altering existing IDE behavior.

Closes arduino#2642

Signed-off-by: dankeboy36 <dankeboy36@gmail.com>
Signed-off-by: dankeboy36 <dankeboy36@gmail.com>
Signed-off-by: dankeboy36 <dankeboy36@gmail.com>
Signed-off-by: dankeboy36 <dankeboy36@gmail.com>
Copy link
Collaborator

@giacomocusinato giacomocusinato left a comment

Choose a reason for hiding this comment

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

Thank for the PR, it all makes sense barring one minor change 👍

Copy link
Collaborator

@giacomocusinato giacomocusinato left a comment

Choose a reason for hiding this comment

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

We need to also add libsecret-1-dev as a dependency for Linux tasks to fix the build.
I'm happy to create a new PR by cherry picking your commits or we can update this one; same for me!

dankeboy36 and others added 6 commits February 27, 2025 18:18
Signed-off-by: dankeboy36 <dankeboy36@gmail.com>
Signed-off-by: dankeboy36 <dankeboy36@gmail.com>
Signed-off-by: dankeboy36 <dankeboy36@gmail.com>
+ event emitter dispatches the new state.

Signed-off-by: dankeboy36 <dankeboy36@gmail.com>
Signed-off-by: dankeboy36 <dankeboy36@gmail.com>
@dankeboy36
Copy link
Contributor Author

I'm happy to create a new PR by cherry picking your commits

I liked your solution (b11bde1) and cherry-picked it here. Let me know if it's OK for you.

@giacomocusinato
Copy link
Collaborator

Let's keep the original solution you proposed, it's better for long-term support instead of pinning ubuntu version (see discussion here).

I've pushed the changes in a separate PR so we can better keep track of them #2647
Let's wait for #2647 to be merged. The rest of the PR is fine by me 👍

Copy link
Collaborator

@giacomocusinato giacomocusinato left a comment

Choose a reason for hiding this comment

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

Thanks 🙏

@dankeboy36
Copy link
Contributor Author

Thank you for reviewing the PR multiple times. If it looks good, can you please merge it?

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.

Make the Arduino state update (for extensions) independent of the language server availability
2 participants