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

misc(scripts): check for UIStrings text before using require #12722

Closed
wants to merge 1 commit into from

Conversation

connorjclark
Copy link
Collaborator

minor change, needed to play nice with standalone.js in #12702 (requiring causes script to error b/c document, etc. does not exist)

@connorjclark connorjclark requested a review from a team as a code owner June 29, 2021 22:50
@connorjclark connorjclark requested review from patrickhulce and removed request for a team June 29, 2021 22:50
@google-cla google-cla bot added the cla: yes label Jun 29, 2021
// No UIStrings found in the file text or exports, so move to the next.
if (!exportedUIStrings) continue;

throw new Error('UIStrings exported but no definition found');
Copy link
Member

@brendankenny brendankenny Jun 29, 2021

Choose a reason for hiding this comment

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

I know I suggested this, so, my fault, but I think we don't want to get rid of this error because it catches non-standard UIStrings declarations that won't parse with tsc, and without it I believe you won't have any warning about your mistake (except if you notice that en-US.json wasn't updated in the commit diff).

Should we just add some form of standalone to the ignoredPathComponents?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just add some form of standalone to the ignoredPathComponents?

+1 if the goal here is to make this script work for standalone report only

I see a future in which we want to have this script work on files that work in the browser though in which case we need a longer term answer for both concerns.

Alternatives:

  • we have two UIStrings regex, one that's simpler for any UIStrings use, and then our current definition one. We only skip import if there's no mention of UIStrings, period.
  • we shim browser dependencies necessary to be able to import the keys at least

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.

4 participants