-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: message types being incorrect #6414
Conversation
Hmm I don't know how to handle Going to leave it broken for now while other people give opinions. |
73d1147
to
887df2e
Compare
Should be good for a look @gonfunko ! |
887df2e
to
18640c9
Compare
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.
discussed offline, summary:
- test that setLocale actually works (not just the type checking passes, but the page is actually in french) (this should probably be an automated test in the future)
- test that messages still work in script-tag scenario
- add comment that setLocale is useless in script-tag scenario
- file a bug for follow up to possibly improve this type. in the current type you'll get type errors if using custom translations. we can maybe get the best of both worlds by using a record type where you can specify specific strings, and then maybe we can add a union with just
string
type to account for custom translations, and that would hopefully still give us autocomplete for the built in ones. https://www.typescriptlang.org/docs/handbook/utility-types.html#recordkeys-type
f4b0561
to
f15befb
Compare
Just waiting for the CI to pass, then this is gtg. |
The basics
npm run format
andnpm run lint
The details
Resolves
Work on #6358
Proposed Changes
Fixes the message files not being typed, and setLocale not being typed.
Behavior Before Change
Type errors.
Behavior After Change
No type errors!
Reason for Changes
Type errors are sad.
Test Coverage
Linked to samples and tested that type errors did not occur when doing:
Documentation
N/A (I think this is how we currently tell developers to import the message definitions).
Additional Information
Ideally @maribethb would be able to look at this since she dug into trying to type the message files before. But she's not around, so I might try to dig up anything she commited wrt this before. For now, just wanted to put up what I had so far.