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.

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

Set basename in order to allow the deployment of MFE in paths #113

Merged
merged 1 commit into from
Oct 22, 2020
Merged

Set basename in order to allow the deployment of MFE in paths #113

merged 1 commit into from
Oct 22, 2020

Conversation

morenol
Copy link
Contributor

@morenol morenol commented Oct 1, 2020 •
edited
Loading

For proper usage in the devstack are needed the changes from openedx/frontend-build#119 and from openedx/frontend-build#131

In this image I am serving the frontend-app-learning MFE in the devstack in the /learning/ path:

image

@openedx-webhooks
Copy link

openedx-webhooks commented Oct 1, 2020 •
edited
Loading

Thanks for the pull request, @morenol! I've created OSPR-5027 to keep track of it in JIRA, where we prioritize reviews. 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 1, 2020
@natabene
Copy link

natabene commented Oct 1, 2020

@morenol Thank you for your contribution. Please let me know once this is ready for our review (you can also just change it from Draft and remove [WIP] in the title).

davidjoy
davidjoy reviewed Oct 5, 2020
View reviewed changes
src/initialize.js Outdated
export const history = createBrowserHistory();

export const history = createBrowserHistory({
basename: process.env.BASENAME || '/',
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be able to use getConfig() here, rather than the process.env variable. We avoid using process.env directly in the code so that we can easily switch out the configuration method/mock it/etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially did that (you can see the first commit of this PR) and it worked (and that is what I used to deploy using paths.) but in the tests that is failing. It says that getConfig() is undefined. Not sure what I have to do in order to fix these tests

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's a circular import... Ugh, that's probably it. initialize.js imports getConfig from config.js, and config.js imports some constants from initialize.js. Since history is evaluated as the file is parsed, getConfig isn't defined yet. Something like that.

An easy way to see if this is the problem with the tests would be to temporarily stop importing APP_CONFIG_INITIALIZED in config.js and just dump its string value in ensureConfig instead - I bet the test starts passing then.

If you're okay with a minor refactoring, I think the right move may be to move the constants (APP_TOPIC, APP_CONFIG_INITIALIZED, etc.) out of initialize.js into a constants.js file, which would keep config.js from needing to import initialize.js. The index.js file would then re-export them from constants.js instead. There may be other consumers that might need their imports updated as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @davidjoy, As you suggested, I created a constant.js file. Let me know if you think that it is Ok

@morenol morenol force-pushed the lmm/use_basename branch 4 times, most recently from f19a279 to a999d26 Compare October 5, 2020 18:50
morenol
morenol commented Oct 5, 2020
View reviewed changes
src/react/AppProvider.jsx Outdated
@@ -62,7 +63,7 @@ export default function AppProvider({ store, children }) {
>
<IntlProvider locale={locale} messages={getMessages()}>
<OptionalReduxProvider store={store}>
<Router history={history}>
<Router history={history} basename={getConfig().BASENAME}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will have to use this, once we migrate to React-router v6

https://github.com/ReactTraining/react-router/blob/dev/docs/advanced-guides/migrating-5-to-6.md#move-basename-from-router-to-routes

@morenol morenol mentioned this pull request Oct 10, 2020
Merged
@openedx-webhooks openedx-webhooks added awaiting prioritization engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. awaiting prioritization labels Oct 13, 2020
@morenol morenol changed the title [WIP] Set basename in order to allow the deployment of MFE in paths Set basename in order to allow the deployment of MFE in paths Oct 19, 2020
@morenol morenol marked this pull request as ready for review October 19, 2020 21:07
@morenol
Copy link
Contributor Author

morenol commented Oct 19, 2020

Hi @natabene, this is now ready for review.

@natabene
Copy link

@davidjoy Please review whenever you have a chance.

@davidjoy
Copy link
Contributor

davidjoy commented Oct 22, 2020 •
edited
Loading

@morenol Hi - heard this came up at the contributors meeting this morning! An update from my end - I'm working on a related PR to update the version of frontend-build in this repo, which has taken me a little time to pull together. It hit a snag because frontend-platform was 3 major versions behind on its frontend-build dependency, which caused all sorts of linting/test errors which I've almost worked through. This PR looks good - I'm planning on merging it as soon as I have a companion PR ready. (It was enough of a pain to upgrade that I decided I should just do it since I had context on the 3 major versions I had to upgrade through)

We're close!

@davidjoy
Copy link
Contributor

@morenol The companion PR I mentioned: #122

I'm having that reviewed internally, so only bother to look if you're curious about linting/test changes. :) It should otherwise have little bearing on this PR.

Merging shortly...

@davidjoy davidjoy merged commit 68b2ad2 into openedx:master Oct 22, 2020
@openedx-webhooks
Copy link

@morenol 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

This was referenced Nov 9, 2020
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidjoy davidjoy davidjoy left review comments

Assignees
No one assigned
Labels
merged open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@morenol @openedx-webhooks @natabene @davidjoy