feat: Initial changes to support OEP65 #535
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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. |
mphilbrick211
added
the
needs test run
|
@pedromartello looks like there are some test failures. I realize this is still draft, but wanted to mention. |
e0d
removed
the
needs test run
@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.
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
configureI18nstanza, 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.
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.
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.
@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-intldependency to6.4.4. The previous version5.25.1is incompatible with Piral and generated errors in the React component hierarchy.reduxto8.1.1. The previous version7.1.1is 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/paragonto^21.1.2to resolve dependency conflicts with the versions ofreact-intlandreact-reduxabove