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

fix: message types being incorrect #6414

Merged
merged 7 commits into from
Sep 26, 2022
Merged

Conversation

BeksOmega
Copy link
Collaborator

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm 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:

import * as  Blockly from 'blockly/core';
import * as Fr from 'blockly/msg/fr';

Blockly.setLocale(Fr);
console.log(Fr.ADD_COMMENT);

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.

@BeksOmega
Copy link
Collaborator Author

Hmm I don't know how to handle setLocale. It definitely feels like it should be defined in blockly.ts (because it's defined in both the core and browser targets) but obviously the build isn't happy with this setup.

Going to leave it broken for now while other people give opinions.

@BeksOmega
Copy link
Collaborator Author

Should be good for a look @gonfunko !

Copy link
Contributor

@maribethb maribethb left a comment

Choose a reason for hiding this comment

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

discussed offline, summary:

  1. 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)
  2. test that messages still work in script-tag scenario
  3. add comment that setLocale is useless in script-tag scenario
  4. 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

@BeksOmega
Copy link
Collaborator Author

  1. Done! French actually works
  2. Done! Loading from script tags works.
  3. Done! Comment added.
  4. Filed Improve types for locales #6452

Just waiting for the CI to pass, then this is gtg.

@BeksOmega BeksOmega merged commit 9c81e75 into google:develop Sep 26, 2022
@BeksOmega BeksOmega deleted the fix/msg-types branch October 4, 2022 18:13
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

3 participants