Fix trap handling when running components #382
Merged
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
elliottt
force-pushed
the
trevor/component-trap-test
branch
from
9dd8d53 to
3b35f22
Compare
acfoltzer
reviewed
Jun 13, 2024
acfoltzer
suggested changes
Jun 13, 2024
Comment on lines
+15
to
+22
|
|
||
| trappable_error_type: { | ||
| "fastly:api/types/error" => types::TrappableError, | ||
| }, | ||
|
|
||
| trappable_imports: [ | ||
| "header-values-get" | ||
| ], |
There was a problem hiding this comment.
It's a little funky that these types will be different even in the non-trapping config, but I think this probably the lightest touch possible for making the two modes work out (vs duplicating the macro invocation entirely between both configs... ugh)
There was a problem hiding this comment.
It also makes it so that we don't have to deal with nested Result values in the return type for header-values-get, which is much easier for me to think about 😅
elliottt
force-pushed
the
trevor/component-trap-test
branch
from
1459351 to
5bde097
Compare
GeeWee
pushed a commit
to GeeWee/Viceroy
that referenced
this pull request
Jul 25, 2024
The component implementation didn't have support for either the test-fatalerror-config feature, or the ability to treat the Error::FatalError variant as a trap. This PR fixes that by doing the following: * Allowing header_values_get to trap based on the test-fatalerror-config feature * Introducing the TrappableError type for treating FatalError as a trap * Adding a variant of the trap-test that uses the component adapter The remaining difference to the wiggle implementation is that only functions listed in trappable_errors will be able to emit traps for FatalError, though the only case we currently use that in is for test-fatalerror-config, so it seemed reasonable to not switch everything to TrappableError.
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.
The component implementation didn't have support for either the
test-fatalerror-configfeature, or the ability to treat theError::FatalErrorvariant as a trap. This PR fixes that by doing the following:header_values_getto trap based on thetest-fatalerror-configfeatureTrappableErrortype for treatingFatalErroras a traptrap-testthat uses the component adapterThe remaining difference to the wiggle implementation is that only functions listed in
trappable_errorswill be able to emit traps forFatalError, though the only case we currently use that in is fortest-fatalerror-config, so it seemed reasonable to not switch everything toTrappableError.