Skip to content

refactor: migrated off babel-plugin-react-intl #258

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jan 20, 2023
Merged

refactor: migrated off babel-plugin-react-intl #258

merged 8 commits into from
Jan 20, 2023

Conversation

BilalQamar95
Copy link
Contributor

Ticket

120: Migrate off babel-plugin-react-intl in favor of babel-plugin-formatjs

What has changed

  • Migrated to babel-plugin-formatjs

@BilalQamar95 BilalQamar95 self-assigned this Aug 24, 2022
@BilalQamar95 BilalQamar95 mentioned this pull request Nov 1, 2022
Closed
1 task
@BilalQamar95 BilalQamar95 requested a review from a team November 1, 2022 05:29
@arbrandes arbrandes linked an issue Nov 1, 2022 that may be closed by this pull request
Migrate off babel-plugin-react-intl in favor of babel-plugin-formatjs openedx/wg-frontend#120
Closed
1 task
@arbrandes arbrandes mentioned this pull request Nov 1, 2022
Closed
30 tasks
arbrandes
arbrandes reviewed Nov 10, 2022
View reviewed changes
Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Have a couple of questions: think you can shed some light on them?

config/babel.config.js Outdated
'formatjs',
{
messagesDir: './temp/babel-plugin-react-intl',
moduleSourceName: '@edx/frontend-platform/i18n',
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like moduleSourceName was removed in v9.0.0. Can you confirm we really don't need to do anything else in addition to just removing the line?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes this one seems extra, i think we can safely remove it

@BilalQamar95
Copy link
Contributor Author

BilalQamar95 commented Nov 11, 2022 •
edited
Loading

@arbrandes Hi, you raised some valid points. Now that we are more familiar with frontend-build & its dependencies I think we would have to revisit this task in light of frontend-build consumers (frontend-platform specifically where I believe this could cause problems) keeping in mind the breaking changes that could be introduced with the upgrade.

@arbrandes arbrandes mentioned this pull request Nov 14, 2022
Closed
36 tasks
@arbrandes arbrandes mentioned this pull request Nov 29, 2022
Closed
28 tasks
@arbrandes
Copy link
Contributor

@BilalQamar95, sprint check: have you been able to revisit this one?

@BilalQamar95
Copy link
Contributor Author

@BilalQamar95, sprint check: have you been able to revisit this one?

Yes, @abdullahwaheed did the initial research work on it & replied to your comments addressing the highlighted concerns. I will update the PR shortly to remove moduleSourceName & resolve conflicts

@adamstankiewicz adamstankiewicz mentioned this pull request Dec 21, 2022
Closed
@arbrandes arbrandes mentioned this pull request Dec 21, 2022
Closed
22 tasks
@abdullahwaheed
Copy link
Contributor

@adamstankiewicz / @arbrandes could you please review it again

arbrandes
arbrandes requested changes Dec 22, 2022
View reviewed changes
Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

I think we missed a spot, but in principle looks good. We can merge after addressing that comment.

@arbrandes arbrandes mentioned this pull request Jan 18, 2023
Closed
44 tasks
arbrandes
arbrandes approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

+1

@arbrandes arbrandes changed the title Migrate off babel-plugin-react-intl refactor: migrated off babel-plugin-react-intl Jan 20, 2023
@arbrandes arbrandes merged commit ca7ae20 into master Jan 20, 2023
@arbrandes arbrandes deleted the bilalqamar95/migrate-off-react-intl-plugin branch January 20, 2023 17:13
@edx-semantic-release
Copy link

🎉 This PR is included in version 12.4.16 🎉

The release is available on:

Your semantic-release bot 📦🚀

@robrap robrap mentioned this pull request Jan 23, 2023
Closed
2 tasks
@robrap
Copy link
Contributor

robrap commented Jan 23, 2023

[inform] Not sure if this caused translation failures. See edx/frontend-component-header-edx#338. Thank you.

@BilalQamar95 BilalQamar95 mentioned this pull request Jan 25, 2023
Merged
@abdullahwaheed abdullahwaheed restored the bilalqamar95/migrate-off-react-intl-plugin branch February 16, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arbrandes arbrandes arbrandes approved these changes

@adamstankiewicz adamstankiewicz Awaiting requested review from adamstankiewicz

+1 more reviewer

@abdullahwaheed abdullahwaheed abdullahwaheed left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@BilalQamar95 BilalQamar95

Labels

needs triage released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate off babel-plugin-react-intl in favor of babel-plugin-formatjs

5 participants

@BilalQamar95 @arbrandes @abdullahwaheed @edx-semantic-release @robrap