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

chore: ugrade to react-intl v4 #1533

Merged
merged 4 commits into from
Jun 5, 2020
Merged

chore: ugrade to react-intl v4 #1533

merged 4 commits into from
Jun 5, 2020

Conversation

emmenko
Copy link
Member

@emmenko emmenko commented May 28, 2020

I tried things locally and I didn't get any error this time. So maybe "it just works" now?

Eventually, we can also remove babel-plugin-macros. I don't know of any use case we have that require the macro plugin.

@changeset-bot
Copy link

changeset-bot bot commented May 28, 2020

🦋 Changeset is good to go

Latest commit: 93dd8f6

We got this.

This PR includes changesets to release 9 packages
Name Type
merchant-center-application-template-starter Minor
@commercetools-frontend/application-components Minor
@commercetools-frontend/application-shell Minor
@commercetools-frontend/mc-scripts Minor
@commercetools-frontend/react-notifications Minor
playground Minor
@commercetools-local/visual-testing-app Minor
@commercetools-website/custom-applications Minor
@commercetools-website/components-playground Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@emmenko emmenko requested a review from tdeekens May 28, 2020 17:30
@vercel
Copy link

vercel bot commented May 28, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/6fh5pnvol
✅ Preview: https://merchant-center-application-kit-git-nm-react-intl-v4.commercetools.now.sh

@tdeekens
Copy link
Contributor

Eventually, we can also remove babel-plugin-macros. I don't know of any use case we have that require the macro plugin.

I can't recall us using it. Maybe we wanted to use it for GraphQL or the rule buider.

@vercel vercel bot temporarily deployed to Preview May 28, 2020 18:56 Inactive
@tdeekens
Copy link
Contributor

@emmenko can we actually say what's breaking on our end here towards consumers? It's late, maybe I don't see it :)

@emmenko
Copy link
Member Author

emmenko commented May 28, 2020

The version bump of the peer dependency is what makes it breaking.

@tdeekens
Copy link
Contributor

Just didn’t see the peer dep bump. Thought we could „claim“ we just work with v3 or v4.

@emmenko
Copy link
Member Author

emmenko commented Jun 3, 2020

In theory yes, but we also don't know what features of v3 users are using. So only supporting v4 seems a safer bet.

I don't have a super strong opinion though.

@tdeekens
Copy link
Contributor

tdeekens commented Jun 3, 2020

Do we have a list of potential features breaking? We could also add it as a range of 3.x || 4.x for now if possible.

@tdeekens
Copy link
Contributor

tdeekens commented Jun 5, 2020

@emmenko any input regarding my previous comment ⏫.

@emmenko
Copy link
Member Author

emmenko commented Jun 5, 2020

Here are the documented changes: https://formatjs.io/docs/react-intl/upgrade-guide-4x

Do you prefer to keep the range then?

@tdeekens
Copy link
Contributor

tdeekens commented Jun 5, 2020

Do you prefer to keep the range then?

What I thought was to allow users to use both 3.x and 4.x. What they use then depends on how their messages should look like and what's allowed. Instead of forcing them to migrate to the newer version which we don't have to require.

Does that make sense? Maybe I am reading/read the release notes wrong.

@vercel vercel bot temporarily deployed to Preview June 5, 2020 11:01 Inactive
@emmenko emmenko marked this pull request as ready for review June 5, 2020 11:01
@emmenko emmenko requested review from tdeekens and adnasa June 5, 2020 11:01
@adnasa adnasa requested review from ahmehri and removed request for adnasa June 5, 2020 11:07
@adnasa
Copy link
Contributor

adnasa commented Jun 5, 2020

Adding @ahmehri to the PR as he is refactoring routing.

@ahmehri
Copy link
Member

ahmehri commented Jun 5, 2020

@adnasa I couldn't find the relationship between this PR and the refactoring of routing, can you elaborate on that?

@adnasa
Copy link
Contributor

adnasa commented Jun 5, 2020

I never checked the file changes so I just assumed they are related.
but I think the main reason I change assignee is because my attention is needed else where and you attempted to upgrade this yourself. so it's worth looking into?

but if you don't want to review, all good. I'll do it

@emmenko emmenko merged commit 96ab311 into master Jun 5, 2020
@emmenko emmenko deleted the nm-react-intl-v4 branch June 5, 2020 19:21
@github-actions github-actions bot mentioned this pull request Jun 5, 2020
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.

4 participants