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

[BUGFIX beta] improve fn & on undefined callback message #18353

Merged
merged 1 commit into from
Apr 13, 2020
Merged

[BUGFIX beta] improve fn & on undefined callback message #18353

merged 1 commit into from
Apr 13, 2020

Conversation

patricklx
Copy link
Contributor

include parent and property key in the message

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Generally seems good, I think that CI is failing due to some testing the exact assertion message. Also, in a recent PR @chancancode added a ref.debug method that we might be able to leverage here instead of manually digging into callbackRef.propertyKey.

@patricklx
Copy link
Contributor Author

Hi, I was still not quite happy with the debug function since it gave me not eough information on where the error is.
But I found there is a debug stack for the templates, So I add that now to the error message.
So now we would have a template stack + helper debug message.

Copy link
Contributor

@pzuraq pzuraq left a comment

Choose a reason for hiding this comment

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

Can you provide a description of what information this adds to the debug output? It seems to be serializing the full debug stack and adding it to any errors that occur, it'd be useful to see an example of what the output looks like to determine how helpful it would be

@rwjblue
Copy link
Member

rwjblue commented Sep 10, 2019

@chancancode would mind taking a look at this one also? It seems related to recent work that you've done...

@patricklx
Copy link
Contributor Author

patricklx commented Sep 10, 2019

Hi, the description will look like this:

Uncaught Error: Assertion Failed: You must pass a function as the `fn` helpers first argument, you passed undefined
  in -> component: ui/routes/posts/details/posts-details
       ⤤ template: client/src/ui/routes/posts/details/template.hbs
       ⤤ template: packages/@ember/-internals/glimmer/lib/templates/outlet.hbs
       ⤤ template: client/src/ui/routes/application/template.hbs
       ⤤ template: packages/@ember/-internals/glimmer/lib/templates/outlet.hbs
    at assert (index.js:181)
    at fnHelper (index.js:7136)
    at InternalHelperReference.compute (index.js:855)
    at InternalHelperReference.value (index.js:400)
    at CapturedNamedArguments.value (runtime.js:5192)
    at processComponentArgs (index.js:4585)
    at CurlyComponentManager.create (index.js:4814)
    at Object.evaluate (runtime.js:1594)
    at AppendOpcodes.evaluate (runtime.js:94)
    at LowLevelVM.evaluateSyscall (runtime.js:3530)

unfortunately it doesnt work for modifiers

@rwjblue
Copy link
Member

rwjblue commented Sep 19, 2019

An update recently landed (in #18372) to add a new type of render debug support (this was needed for supporting custom component types in the ember-inspector), I'd be interested to see if you can take a look at that work and leverage it for some of the plumbing here.

@rwjblue
Copy link
Member

rwjblue commented Sep 25, 2019

Looks great to me, would you mind squashing the commits down to one and prefixing it with [BUGFIX beta] (so we can make sure to get this into the 3.14 release)?

@patricklx patricklx changed the title improve fn undefined callback message [BUGFIX beta] improve fn & on undefined callback message Sep 26, 2019
@James1x0
Copy link

James1x0 commented Apr 8, 2020

🙏Bitten by this often migrating to new syntaxes in octane. Any update @rwjblue ?

@rwjblue rwjblue merged commit f36a2cc into emberjs:master Apr 13, 2020
@rwjblue
Copy link
Member

rwjblue commented Apr 13, 2020

Thanks again @patricklx!

@deanpapastrat
Copy link

This is a godsend. Thank you for this 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants