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

Improved Alpine error resiliency and logging #2027

Merged
merged 17 commits into from Oct 11, 2021

Conversation

blackmagic0
Copy link
Contributor

  • Improve Alpine resiliency by preventing Alpine from locking up when an error is reached ( which would stop other valid components from running initTree )
  • Errors log the cause element and expression making debugging easier (same as V2 did)
  • Errors that are thrown are always assigned the element and expression (same as V2 did)
  • Add Cypress configuration to test utils to assert the element and expression are on any error/log coming from Alpine and test range of errors
  • Fix store test that had reference error in it
  • Fix custom directives test that had reference errors in it

Resiliency example

Without these changes: https://codepen.io/danddanddand/pen/QWgGeZB
With these changes: https://codepen.io/danddanddand/pen/eYRBqLo

@Pronian
Copy link

Pronian commented Sep 24, 2021

Hello! Does anyone know what needs to happen in order to have this merged? I really want this feature.

@HugoDF
Copy link
Contributor

HugoDF commented Oct 8, 2021

@Pronian I think it's pending a @calebporzio review 😄

The previous PR with similar changes had a bunch of discussion on it so he asked for it to be re-created with a clear description to make it easier to review.

@@ -70,7 +70,7 @@ test('store\'s "this" context is reactive for init function',
[html`
<div x-data>
<span x-text="$store.test.count"></span>
<button @click="$store.test.increment()" id="button">increment</button>
<button id="button">increment</button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose for removing the @click here?

Copy link
Contributor

Choose a reason for hiding this comment

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

increment function doesn't exist so test fails with reference error once error handling implemented

@@ -26,7 +26,6 @@ test('directives are auto cleaned up',
`,
`
Alpine.directive('foo', (el, {}, { effect, cleanup, evaluateLater }) => {
let evaluate = evaluateLater('foo')
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of removing these lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing. Expression 'foo' is evaluated and since it's invalid test fails once error handling is happening.

@calebporzio
Copy link
Collaborator

Thanks for the improvement on this PR and sorry for lagging on it.

Things look mostly good. I left a review inquiring about those seemingly unrelated test modifications - want to make sure we're not breaking anything.

@calebporzio calebporzio merged commit c5ffa11 into alpinejs:main Oct 11, 2021
@calebporzio
Copy link
Collaborator

Thanks for following up!

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.

None yet

5 participants