-
Notifications
You must be signed in to change notification settings - Fork 143
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
Migrate localization validations to Core #5165
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Coverage report
Show files with reduced coverage 🔻
Test suite run success2000 tests passing in 904 suites. Report generated by 🧪jest coverage report action from f80ea94 |
eb1175a
to
19173f7
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.
Obviously pending the other PR, this looks good to me! There is no code like no code 😁
Please be sure to add a changeset before merging.
// Locale validations | ||
for (const locale of localesPaths) { | ||
const size = fs.statSync(locale).size | ||
if (size > L10N_FILE_SIZE_LIMIT) | ||
throw new AbortError( |
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.
Will users continue to see a validation error if it's too big? Essentially we're just moving the validation from pre to post-upload attempt?
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.
Yes exactly, users will still see the validation, but now they will be coming the post-upload system.
See for context on why we are removing this: https://github.com/Shopify/shopify/pull/566161#issue-2780703768
Checklist