-
Notifications
You must be signed in to change notification settings - Fork 64
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
feat: Initial changes to support OEP65 #535
Conversation
Thanks for the pull request, @pedromartello! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
@pedromartello looks like there are some test failures. I realize this is still draft, but wanted to mention. |
@e0d This is an npm dependency failure: Wondering if @arbrandes has a recommendation as he was looking at a related concern before I issued a PR for EDIT: I marked the related PR in |
@e0d @arbrandes - I moved this PR to "ready for review" as the required changes to paragon have been merged and released. |
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.
No objections in principle, but I think it's best to separate out these changes into different commits:
- One for the redux update, with a message linking to the 8.0.0 changelog (this one) noting that there are no (known) breaking changes
- Another for the react-intl upgrade, with a similar message
- A third one for the
mergeMessages()
change (or new function, depending) - A fourth for the reordering of the
configureI18n
stanza, with an explanation.
These might have all been separate PRs, but I don't think we need to go that far unless somebody objects.
return Array.isArray(messagesArray) ? merge({}, ...messagesArray) : {}; | ||
export function mergeMessages(newMessages) { | ||
const msgs = Array.isArray(newMessages) ? merge({}, ...newMessages) : newMessages; | ||
messages = merge(messages, msgs); |
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.
Doing this would change the expected behavior of the function. Do we know if any other packages/MFEs use it?
Maybe it's best to create a new function for this purpose, just in case. What do you think?
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.
see my comment below
@@ -262,7 +265,7 @@ export function configure(options) { | |||
loggingService = options.loggingService; | |||
// eslint-disable-next-line prefer-destructuring | |||
config = options.config; | |||
messages = Array.isArray(options.messages) ? mergeMessages(options.messages) : options.messages; | |||
messages = Array.isArray(options.messages) ? merge({}, ...options.messages) : options.messages; |
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.
@arbrandes - the only use of the mergeMessages function was here (and in the unit tests). Note how it has now been changed to use lodash.merge directly. The exports from @edx/frontend-platform
and @edx/frontend-platform/i18n
were added in this commit.
Closing to follow the suggestions in this comment: #535 (review) |
@pedromartello Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description:
This pull request introduces changes to support the use of Piral the micro-frontend framework used in the prototype for OEP-65 - currently referred to OEPXXXX.
Specific changes introduced:
react-intl
dependency to6.4.4
. The previous version5.25.1
is incompatible with Piral and generated errors in the React component hierarchy.redux
to8.1.1
. The previous version7.1.1
is incompatible with Piral and generated errors where the Redux store was not available during component rendering.mergeMessages
. Previously this method would overwrite existing messages with the new messages. The new version overloads this function to actually merge new messages with existing ones.@edx/paragon
to^21.1.2
to resolve dependency conflicts with the versions ofreact-intl
andreact-redux
above