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.

Sign up for GitHub

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

Jump to bottom

Improve extension loading state and starting #554

Merged
merged 2 commits into from
Jan 19, 2022
Merged

Improve extension loading state and starting #554

merged 2 commits into from
Jan 19, 2022

Conversation

tombruijn
Copy link
Member

@tombruijn tombruijn commented Jan 18, 2022 •
edited
Loading

The problem

The Extension's 'isLoaded attribute was only set when the
extension's start function was called. This made it difficult to
determine if the extension was really loaded in the app or not, without
starting the extension first.

This made it necessary to start the extension in scenarios where we need
to call extension functions without needing to starting it first, such
as in the diagnose command.

Calling Extension's start doesn't change any state in the extension
that needs to be set before any other function can be called. I think it
was written this way as a way to communicate loading issues. A better
name for this attribute would have been isRunning, but this is not
something we have to track.

Solution

Update how the isLoaded is set, by checking if the extension was
loaded into the app or not on load, not just when start is called.

I've chosen to communicate this with an export from the extension
wrapper, rather than if the start function returns an error or not.
Since the value doesn't change at runtime it's now a static attribute of
the class.

I've removed the isLoaded return values from the extension's start
and stop function, these are unused.

About the error thrown by start

I've left the Extension's start function intact to print an error
when called and the extension isn't loaded. In other integrations we
print this on load of the extension. I suspect this was done on start,
so that it wouldn't always be printed on unsupported Operating Systems.
We may want to revisit this, or find another way to not print this all
the time for those systems.

Only start extension when its loaded and active

Do not automatically start the extension when it's initialized, but do
this from the client like in all the other integrations. This also makes
it easier to initialize the Extension class for other uses, like the
diagnose without having to worry about the active state or not.

Closes #551

@tombruijn tombruijn self-assigned this Jan 18, 2022
@tombruijn tombruijn marked this pull request as ready for review January 18, 2022 14:45
@tombruijn tombruijn mentioned this pull request Jan 19, 2022
Open
3 tasks
tombruijn added a commit that referenced this pull request Jan 19, 2022
@tombruijn
Fix extension function fallbacks
459e9aa 
As reported in #556, when the extension is not installed or fails to
load, calls to certain extension functions will fail. The issue mentions
the `runningInContainer` broken being broken on the main branch.

PR #554 also removes some safety checks that cause the same issue for
the `diagnoseRaw` function. Those safety checks were removed because
they did not what expected, tracking if the extension was running or
not.

For both functions I've implemented a no-operations function. This will
fix any errors on calls to these functions. Other functions that call
these functions need to handle receiving no return value.

Part of #556, but doesn't fix it entirely. We should call more function
calls. These ones were the most obvious ones.

Based on PR #554.
tombruijn added a commit that referenced this pull request Jan 19, 2022
@tombruijn
Fix extension function fallbacks
7a8f54b 
As reported in #556, when the extension is not installed or fails to
load, calls to certain extension functions will fail. The issue mentions
the `runningInContainer` broken being broken on the main branch.

PR #554 also removes some safety checks that cause the same issue for
the `diagnoseRaw` function. Those safety checks were removed because
they did not what expected, tracking if the extension was running or
not.

For both functions I've implemented a no-operations function. This will
fix any errors on calls to these functions. Other functions that call
these functions need to handle receiving no return value.

Part of #556, but doesn't fix it entirely. We should call more function
calls. These ones were the most obvious ones.

Based on PR #554.
@tombruijn tombruijn mentioned this pull request Jan 19, 2022
Merged
tombruijn added a commit that referenced this pull request Jan 19, 2022
@tombruijn
Fix extension function fallbacks
3523971 
As reported in #556, when the extension is not installed or fails to
load, calls to certain extension functions will fail. The issue mentions
the `runningInContainer` broken being broken on the main branch.

PR #554 also removes some safety checks that cause the same issue for
the `diagnoseRaw` function. Those safety checks were removed because
they did not what expected, tracking if the extension was running or
not.

For both functions I've implemented a no-operations function. This will
fix any errors on calls to these functions. Other functions that call
these functions need to handle receiving no return value.

Part of #556, but doesn't fix it entirely. We should call more function
calls. These ones were the most obvious ones.

Based on PR #554.
tombruijn added a commit that referenced this pull request Jan 19, 2022
@tombruijn
Fix extension function fallbacks
c62798c 
As reported in #556, when the extension is not installed or fails to
load, calls to certain extension functions will fail. The issue mentions
the `runningInContainer` broken being broken on the main branch.

PR #554 also removes some safety checks that cause the same issue for
the `diagnoseRaw` function. Those safety checks were removed because
they did not what expected, tracking if the extension was running or
not.

For both functions I've implemented a no-operations function. This will
fix any errors on calls to these functions. Other functions that call
these functions need to handle receiving no return value.

Part of #556, but doesn't fix it entirely. We should call more function
calls. These ones were the most obvious ones.

Based on PR #554.
@tombruijn tombruijn changed the base branch from main to fix-ci-test-failures January 19, 2022 09:57
@tombruijn
Copy link
Member Author

Rebased on PR #558 because the failure scenario tests were not run.

tombruijn added a commit that referenced this pull request Jan 19, 2022
@tombruijn
Fix extension function fallbacks
10cfc29 
As reported in #556, when the extension is not installed or fails to
load, calls to certain extension functions will fail. The issue mentions
the `runningInContainer` broken being broken on the main branch.

PR #554 also removes some safety checks that cause the same issue for
the `diagnoseRaw` function. Those safety checks were removed because
they did not what expected, tracking if the extension was running or
not.

For both functions I've implemented a no-operations function. This will
fix any errors on calls to these functions. Other functions that call
these functions need to handle receiving no return value.

Part of #556, but doesn't fix it entirely. We should call more function
calls. These ones were the most obvious ones.

Based on PR #554.
@tombruijn tombruijn force-pushed the fix-ci-test-failures branch from 908d079 to 5a0eef9 Compare January 19, 2022 10:08
tombruijn added a commit that referenced this pull request Jan 19, 2022
@tombruijn
Fix extension function fallbacks
5a63030 
As reported in #556, when the extension is not installed or fails to
load, calls to certain extension functions will fail. The issue mentions
the `runningInContainer` broken being broken on the main branch.

PR #554 also removes some safety checks that cause the same issue for
the `diagnoseRaw` function. Those safety checks were removed because
they did not what expected, tracking if the extension was running or
not.

For both functions I've implemented a no-operations function. This will
fix any errors on calls to these functions. Other functions that call
these functions need to handle receiving no return value.

Part of #556, but doesn't fix it entirely. We should call more function
calls. These ones were the most obvious ones.

Based on PR #554.
@tombruijn tombruijn force-pushed the fix-ci-test-failures branch from 5a0eef9 to 6e041c4 Compare January 19, 2022 10:15
tombruijn added a commit that referenced this pull request Jan 19, 2022
@tombruijn
Fix extension function fallbacks
df91f1c 
As reported in #556, when the extension is not installed or fails to
load, calls to certain extension functions will fail. The issue mentions
the `runningInContainer` broken being broken on the main branch.

PR #554 also removes some safety checks that cause the same issue for
the `diagnoseRaw` function. Those safety checks were removed because
they did not what expected, tracking if the extension was running or
not.

For both functions I've implemented a no-operations function. This will
fix any errors on calls to these functions. Other functions that call
these functions need to handle receiving no return value.

Part of #556, but doesn't fix it entirely. We should call more function
calls. These ones were the most obvious ones.

Based on PR #554.
tombruijn added a commit that referenced this pull request Jan 19, 2022
@tombruijn
Fix extension function fallbacks
b542389 
As reported in #556, when the extension is not installed or fails to
load, calls to certain extension functions will fail. The issue mentions
the `runningInContainer` broken being broken on the main branch.

PR #554 also removes some safety checks that cause the same issue for
the `diagnoseRaw` function. Those safety checks were removed because
they did not what expected, tracking if the extension was running or
not.

For both functions I've implemented a no-operations function. This will
fix any errors on calls to these functions. Other functions that call
these functions need to handle receiving no return value.

Part of #556, but doesn't fix it entirely. We should call more function
calls. These ones were the most obvious ones.

Based on PR #554.
@tombruijn
Improve extension loading state and starting
56fb794 
## The problem

The `Extension`'s '`isLoaded` attribute was only set when the
extension's `start` function was called. This made it difficult to
determine if the extension was really loaded in the app or not, without
starting the extension first.

This made it necessary to start the extension in scenarios where we need
to call extension functions without needing to starting it first, such
as in the diagnose command.

Calling `Extension`'s `start` doesn't change any state in the extension
that needs to be set before any other function can be called. I think it
was written this way as a way to communicate loading issues. A better
name for this attribute would have been `isRunning`, but this is not
something we have to track.

## Solution

Update how the `isLoaded` is set, by checking if the extension was
loaded into the app or not on load, not just when `start` is called.

I've chosen to communicate this with an export from the extension
wrapper, rather than if the start function returns an error or not.
Since the value doesn't change at runtime it's now a static attribute of
the class.

I've removed the `isLoaded` return values from the extension's `start`
and `stop` function, these are unused.

## About the error thrown by `start`

I've left the `Extension`'s `start` function intact to print an error
when called and the extension isn't loaded. In other integrations we
print this on load of the extension. I suspect this was done on start,
so that it wouldn't always be printed on unsupported Operating Systems.
We may want to revisit this, or find another way to not print this all
the time for those systems.

## Only start extension when its loaded and active

Do not automatically start the extension when its initialized, but do
this from the client like in all the other integrations. This also makes
it easier to initialize the `Extension` class for other uses, like the
diagnose without having to worry about the active state or not.

Closes #551
tombruijn added a commit that referenced this pull request Jan 19, 2022
@tombruijn
Fix extension function fallbacks
93fdc3a 
As reported in #556, when the extension is not installed or fails to
load, calls to certain extension functions will fail. The issue mentions
the `runningInContainer` broken being broken on the main branch.

PR #554 also removes some safety checks that cause the same issue for
the `diagnoseRaw` function. Those safety checks were removed because
they did not what expected, tracking if the extension was running or
not.

For both functions I've implemented a no-operations function. This will
fix any errors on calls to these functions. Other functions that call
these functions need to handle receiving no return value.

Part of #556, but doesn't fix it entirely. We should call more function
calls. These ones were the most obvious ones.

Based on PR #554.
@tombruijn tombruijn changed the base branch from fix-ci-test-failures to main January 19, 2022 10:25
@tombruijn
Fix extension function fallbacks (#557)
f52a824 
As reported in #556, when the extension is not installed or fails to
load, calls to certain extension functions will fail. The issue mentions
the `runningInContainer` broken being broken on the main branch.

PR #554 also removes some safety checks that cause the same issue for
the `diagnoseRaw` function. Those safety checks were removed because
they did not what expected, tracking if the extension was running or
not.

For both functions I've implemented a no-operations function. This will
fix any errors on calls to these functions. Other functions that call
these functions need to handle receiving no return value.

Part of #556, but doesn't fix it entirely. We should call more function
calls. These ones were the most obvious ones.

Based on PR #554.
@tombruijn tombruijn merged commit 9058948 into main Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luismiramirez luismiramirez luismiramirez approved these changes

@jeffkreeftmeijer jeffkreeftmeijer jeffkreeftmeijer approved these changes

@unflxw unflxw Awaiting requested review from unflxw

Assignees

@tombruijn tombruijn

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extension should be accessible even if active is set to false
3 participants
@tombruijn @jeffkreeftmeijer @luismiramirez