-
Notifications
You must be signed in to change notification settings - Fork 50
265/show ffi errors to user #267
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
265/show ffi errors to user #267
Conversation
client/src/Try/App.purs
Outdated
append (FetchError err) _ = FetchError err | ||
append _ (FetchError err) = FetchError err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this preserves the current behavior for non-FFI errors, since we were using ExceptT
which short-circuits on first error.
data Error | ||
= FetchError String | ||
| FFIErrors (NonEmptyArray String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realizing that I probably want to capture all the errors here, so CompilerErrors
as well, though I'm not sure why those are currently tracked separately from the error type of ExceptT
. For example
trypurescript/client/src/Try/API.purs
Line 101 in 0bd14e2
compile :: forall m. MonadAff m => String -> String -> ExceptT String m (Either String CompileResult) |
String
s (in ExceptT String
and Either String
) and the possibility of CompileResult
to be CompileFailed
.
derive newtype instance functorApp :: Functor m => Functor (AppT m) | ||
derive newtype instance applyApp :: Monad m => Apply (AppT m) | ||
derive newtype instance applicativeApp :: Monad m => Applicative (AppT m) | ||
derive newtype instance bindApp :: Monad m => Bind (AppT m) | ||
derive newtype instance monadApp :: Monad m => Monad (AppT m) | ||
derive newtype instance monadEffectApp :: MonadEffect m => MonadEffect (AppT m) | ||
derive newtype instance monadAffApp :: MonadAff m => MonadAff (AppT m) | ||
derive newtype instance monadThrowApp :: Monad m => MonadThrow Error (AppT m) | ||
|
||
runAppT :: forall m. AppT m ~> ExceptT Error m | ||
runAppT (AppT x) = x | ||
|
||
newtype ParAppT m a = ParAppT (Compose m (V Error) a) | ||
|
||
derive newtype instance functorParApp :: Functor m => Functor (ParAppT m) | ||
derive newtype instance applyParApp :: Apply m => Apply (ParAppT m) | ||
derive newtype instance applicativeParApp :: Applicative m => Applicative (ParAppT m) | ||
|
||
runParAppT :: forall f. ParAppT f ~> Compose f (V Error) | ||
runParAppT (ParAppT x) = x | ||
|
||
instance parallelParAppApp :: Parallel f m => Parallel (ParAppT f) (AppT m) where | ||
parallel = | ||
ParAppT | ||
<<< Compose | ||
<<< map (Either.either V.invalid pure) | ||
<<< Parallel.parallel | ||
<<< ExceptT.runExceptT | ||
<<< runAppT | ||
sequential = | ||
AppT | ||
<<< ExceptT | ||
<<< map (V.toEither) | ||
<<< Parallel.sequential | ||
<<< Newtype.unwrap | ||
<<< runParAppT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this just so I can call parTraverse
and accumulate FFI errors 😄 entirely understandable if we'd rather not add this module and just continue to use ExceptT
& Aff
directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I'll remove this. It was an interesting experiment to enable showing warnings for all missing FFI dependencies while loading in parallel, but it seems like in practice there's rarely a situation where there'd be more than one.
Closing in favor of #268 |
Description of the change
Fixes #265
Clearly and concisely describe the purpose of the pull request. If this PR relates to an existing issue or change proposal, please link to it. Include any other background context that would help reviewers understand the motivation for this PR.
Checklist: