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

Prevent "A message must be provided as a String or AST" error from being throw #1299

Closed
2 tasks done
jacobq opened this issue May 13, 2020 · 12 comments
Closed
2 tasks done

Comments

@jacobq
Copy link
Contributor

jacobq commented May 13, 2020

I still occasionally run into the error message "A message must be provided as a String or AST" with v5.x (addressed by #621?). I realize this is probably my fault (i.e. bug in my app passes bad parameter to t helper or something). However, the app would "still work" (aside from missing some text) if the error did not occur, but the error makes Ember blow up, so I would like to downgrade it to a warning. Is this possible?

Also, are there practical ways to obtain more information about the context of the error? (e.g. what unacceptable value was received, which tool is throwing it [template helper, service API, etc.], etc.)
Historically, when I've hit these before the solution has been trivial (e.g. adding a default like (or foo translationKeyForUnknownValues)), but locating the cause can be a nightmare. Using errorOnMissingTranslations: true doesn't seem to catch dynamic cases (e.g. a "status" value coming from a service).


  • I am on the latest ember-intl version
  • I have searched the issues of this repo and believe that this is not a duplicate

Environment

  • Ember Version: 3.18.1
  • Ember CLI Version: 3.18.0
  • Ember Intl Version: 5.0.9
  • Browser(s): app runs in Electron v8.2.5 (Chromium v80.0.3987.165)
  • Node Version: 12.16.3

Steps to Reproduce

@jasonmit
Copy link
Member

jasonmit commented May 13, 2020

still occasionally run into the error message "A message must be provided as a String or AST" with v5.x (addressed by #621?)

Curious if you can find out what you're passing into it. Maybe we could handle it better. My guess is maybe undefined/null?

Also, are there practical ways to obtain more information about the context of the error? (e.g. what unacceptable value was received, which tool is throwing it [template helper, service API, etc.], etc.)

This is coming from the intl-messageformat compiler. We're passing something right through to it that isn't a string or AST object in this case.

We can very likely catch errors like this and add more details to the thrown error (translation key, params, etc.).

Also, are there practical ways to obtain more information about the context of the error?

We can add more context but if you want to see where it's coming from I recommend pause on exception and walking the callstack back.

@jacobq
Copy link
Contributor Author

jacobq commented May 15, 2020

Curious if you can find out what you're passing into it.

I finally tracked it down. It was undefined. (Edit: I thought that was the case anyway, though I didn't see the same error when I made-up the example in the comment below.) Somehow one of the attributes on one of the models did not get saved in this case. So when a component tried to do something like {{t @someModel.type}} it threw the error. I added some validation / default-merging code for that model and the problem went away.

As I was trying to explain before, typically a single problem like this one isn't hard to fix once identified, but I had to do a lot of digging around to isolate the culprit, so if there is some way I can debug this with DevTools or something I would really like to know. (Maybe it's just me, but most of the time when I look at the stack traces and see glimmer-vm or backburner code I just sigh as I rarely can figure out where my code fits in, even after following the instructions in the infamous "Intrepid developer" comment)

@jacobq
Copy link
Contributor Author

jacobq commented May 15, 2020

We can add more context but if you want to see where it's coming from I recommend pause on exception and walking the callstack back.

I've been putting a breakpoint on my Ember.onerror function, which I think serves the same end (otherwise I have to enable breaking on caught exceptions, which leads to lots of false positives, or turn off my error handler which I'd better not forget to turn back on). But I've had trouble getting insight from this. Everything in the stack trace is from vendor.js (don't see any app code) -- which makes sense if it's happening when Glimmer's doing its thing, I think.

In a super simple case like this one:

// templates/application.hbs
<h1>Hello, world!</h1>
{{t this.somethingNotDefined}}
<h1>Goodbye, world!</h1>

the partially rendered DOM gives a good clue that it happened after "Hello" and before "Goodbye", but in more complex cases (many components, lots of dynamic data, etc.) I haven't found a good way.
image
(and, yes, I know the source maps aren't working -- adopted-ember-addons/ember-electron#442 is killing me)

@jasonmit
Copy link
Member

I can add additional context like "called from t helper" with the key included but if you're passing in undefined as the key I can't really pinpoint in what template that originated.

The helper API does not expose those kinds of details to us.

When passing in undefined as the key I think an error is appropriate since anything less than that would likely mask the bug and make it harder to detect. If you find yourself in this situation often, you could reimplement the t helper to deal with this if a thrown error isn't what you want anymore.

@jacobq
Copy link
Contributor Author

jacobq commented May 15, 2020

Looks like {{t this.foo}} leads to the following messages for the corresponding values of this.foo:

  • null or '' --> Error: Assertion Failed: [ember-intl] translation lookup attempted but no translation key was provided.
  • undefined --> Error: <my-app@helper:t::ember286> helper requires value attribute.
  • true, 1, [], or {} --> TypeError: A message must be provided as a String or AST.

@jacobq
Copy link
Contributor Author

jacobq commented May 15, 2020

I can add additional context like "called from t helper" with the key included but if you're passing in undefined as the key I can't really pinpoint in what template that originated.

95%+ of my text translation is going through t helper in templates, so I'm not sure that would be worth the effort to add. When the stack trace shows glimmer I think that is a strong indicator that the error is occurring while rending a template / using the helper, isn't it? Plus the key / arguments are visible to the debugger at that point, and like you said, if it's an unhelpful value like undefined there's not much to do.

The helper API does not expose those kinds of details to us.

Yeah, I figured, but it seemed worth asking. The "more context" that I was hoping for was, "Which helper?" or "Which component?" or something like that. I suppose I could do something crazy like make my own helper that calls t but includes some tagging/tracking (e.g. name/comment string as first arg), but that sounds a little messy and would still be only a coarse tool.

When passing in undefined as the key I think an error is appropriate since anything less than that would likely mask the bug and make it harder to detect. If you find yourself in this situation often, you could reimplement the t helper to deal with this if a thrown error isn't what you want anymore.

I respectfully disagree as the bug may still be masked (e.g. value isn't undefined in all known cases but then some user has a weird case and encounters it but rather than simply cause some text to break, the rest of the view doesn't render -- #1301). I think it would be far preferable to let the error / warning behavior be configurable via option (e.g. would be awesome to be able to set something in ENV to make it error in dev & test but only log in prod), but that is neither here no there, I suppose.

@jasonmit
Copy link
Member

jasonmit commented May 15, 2020

I respectfully disagree as the bug may still be masked (e.g. value isn't undefined in all known cases but then some user has a weird case and encounters it but rather than simply cause some text to break, the rest of the view doesn't render

This is something you can do in userland by wrapping the {{t}} in a conditional to ensure you're actually passing in a key in a case where it's dynamic.

@jasonmit
Copy link
Member

jasonmit commented May 15, 2020

I've made slight adjustments to include validating key types:
#1302

And now it should fail validating the type on the key and/or not finding the translation key.

I personally prefer to have prod and dev behavior as similar as possible, so I think the idea of not throwing in production builds in this case isn't the best option. Users may not report any issues because things all appear to be fine in the app. Also, it can introduce other side effects in your app only visible in production builds. One example, if you're combining the t helper with another helper/component that might be expecting a string returned.

i.e.,

Now form label is throwing an error and not the t helper
{{form-label label=(t @model.category)}}

@jasonmit
Copy link
Member

jasonmit commented May 15, 2020

I'm going to close this for now and say #1302 will addresses this by making the failed translation key passed in more obvious than failing trying to parse what isn't an AST object.

@sandstrom
Copy link
Contributor

@jacobq @jasonmit We have this issue too!

Translation errors can easily slip in and when throwing exceptions Ember blows up and the page won't render. For example the error here, or The intl string context variable "facet" was not provided to the string "undefined", or any of the other ones that can occur when due to missing translation data or similar.

For us it would be ideal if we could control the outcome, for example via a hook for the common types of errors that can occur.

We could then choose whether we'd like to throw an error (probably in dev) or render a fallback message, "Could not translate "my.translation.key" and then send an XHR to Bugsnag/sentry behind the scenes.

@jacobq
Copy link
Contributor Author

jacobq commented May 15, 2020

Users may not report any issues because things all appear to be fine in the app.

Yes, but they don't have to. The logger could report it back without the user having to do anything (e.g. with TrackJS).

Now form label is throwing an error and not the t helper.

This would not be a problem if t did not throw but rather returned a default ((Could not translate invalid key), ?, ...).

This is something you can do in userland by wrapping the {{t}} in a conditional to ensure you're actually passing in a key in a case where it's dynamic.

OK, this seems like the best solution for me then. (The keys are very often dynamic.) Thanks for the suggestion.

@jasonmit
Copy link
Member

jasonmit commented May 16, 2020

5.1.0 will now throw with a more helpful error indicating you've passed in a value that isn't a String as a translation key. This should avoid some instances where you reach the compiler error: "A message must be provided as a String or AST" which isn't very obvious as to what's going on.

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

No branches or pull requests

3 participants