-
Notifications
You must be signed in to change notification settings - Fork 2
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
Support npm start
on host for MFE development
#25
Comments
|
Update on the discovery/prototyping I've done so far: Restating/clarifying goalsThe main goal I have is parity with the devstack development experience. For example, working on
My feeling is, "if it is more complicated than that to do MFE dev with tutor we have failed." MFE side plan
In this example, I
Tutor side plan
Current issues
tl;drCORS/CSRF are getting in the way of making connecting to services hosted in Tutor from a localhost MFE as easy as it is to connect to services hosted with devstack. Any suggestions on how handle this nicely would be amazing! |
While I'm totally for parity with devstack in terms of developer experience and wholly support your efforts here, and while the current I'm sure there's a simple way we can improve this (and both Tutor and the devstack) by using something like the Edit: yes, even if it means issuing PRs to the devstack, too. |
@brian-smith-tcril Could you refresh me on what the issue is with CORS/CSRF? Is it that LMS & CMS will reject requests originating from host-mode MFEs, which are hosted at |
@kdmccormick I think yes, because by default tutor uses its own hostname for mfe/lms vs devstack which relies on the default localhost. There might be additional, settings, but this of course one of the needed step. |
@kdmccormick I tried that: https://github.com/brian-smith-tcril/tutor-mfe/pull/2/files#diff-9dd8a655b8597bfd6cdb64b96b2a805174561cae462059559fdc4a2b8be0ac06R92-R93, When it didn't work I thought maybe localhost isn't referring to the host machine but instead the container's localhost? I did a little searching but nothing came up |
@brian-smith-tcril You are not specifying the port though, hence https://github.com/overhangio/tutor-mfe/blob/94b058ed559684583b89409f01a5a46549f4c389/tutormfe/patches/openedx-lms-development-settings#L25-L26 |
@brian-smith-tcril yeah, I agree, this seems like overkill, I'm not even sure how we'd do it if we wanted to.
With runtime config, I am optimistic that we will be able to tell MFEs to link to host-served MFEs as appropriate by updating LMS's MFE_CONFIG. Is that what you had in mind as well? |
More or less yeah. I think in some (most? all?) cases we won't even need to modify the The main point of updating config within tutor is ensuring all the correct env vars are set ( |
Right, I was thinking not of tutor-mfe's config dictionary, but rather the MFE_CONFIG dict in LMS settings, which feeds the mfe_config API. Turns out it's just ACCOUNT_PROFILE_URL that would need to be updated there: https://github.com/brian-smith-tcril/tutor-mfe/pull/2/files#diff-9dd8a655b8597bfd6cdb64b96b2a805174561cae462059559fdc4a2b8be0ac06R101
👍🏻 |
So I realized my mistake from before, I had just put |
Woohoo! |
I agree with @arbrandes here. Fortunately, I think runtime config should simplify things. As long as Unfortunately, it looks like |
Ah, I'd forgotten about that! Well, that's one way to go about doing it: instead of using the env files, we use that template with appropriate changes for each/all MFEs. |
We need it for the proxy line. Caddy does this in production, but in dev mode, the only option is (was?) to have the webpack dev server do it. |
So as of right now with devstack, running I'm all for minimizing the amount of hardcoded dev env stuff we have in the MFE repos, and runtime config should go a long way towards this (that way we only need to provide an Any thoughts on how to minimize hardcoded dev environment info in MFE repos while keeping the ability to run locally without requiring manual config? |
@arbrandes @brian-smith-tcril Do you think the user running this would be sufficient? export MFE_CONFIG_API_URL="http://$(tutor config printvalue LMS_HOST)/api/mfe_config/v1"
npm start I know that's a bit of a mouthful, but there are a few ways we could shorten it, for example... Custom CLI command just to print the API URL: MFE_CONFIG_API_URL="$(tutor mfe printconfigurl)" npm start Custom CLI command that prints the path to an env, which can be sourced: . "$(tutor mfe printenv)"
npm start Custom CLI command: tutor npmstart # would set MFE_CONFIG_API_URL appropriately and then run 'npm start' in the working dir EDIT: That last command,
|
We would need to define the path as well |
@kdmccormick the thing that immediately comes to mind with those suggestions is that they require having access to the tutor cli. With that in mind, I think I like . "$(tutor mfe printenv)"
npm start the best. That way devs would only need to be in the venv they have tutor installed in once to generate the config, instead of needing to go into it every time they want to run the MFE. I also think the |
@brian-smith-tcril Unfortunately, running I guess someone could enter a venv and run: $ tutor mfe printenv
~/.local/share/tutor-root/yada/mfe/yada/env.development # for example and then later, from a shell outside of the venv, run: $ . ~/.local/share/tutor-root/yada/mfe/yada/env.development
$ npm start I guess I'm coming from a workflow where I have my tutor venv activated at all times, so the |
I guess I was thinking more of a variation on that command where it creates a My workflow is very much a "only use the venv in the place I need it" one. I activate the venv and start tutor (or devstack), then open a new terminal tab and go into the MFE directory and run I guess the thing I keep coming back to is, why is this https://github.com/openedx/frontend-template-application/blob/master/.env.development ok? If it's not ok, do we remove it from all MFEs and make everyone using devstack for MFE dev run export MFE_CONFIG_API_URL="http://localhost:18000/api/mfe_config/v1"
npm start if it is ok, why can't we just check in a tutor one too? |
Some thoughts on this:
|
Oh, also: This issue is part of the greater "replace Devstack with Tutor" initiative. When we make |
I think that's reasonable! |
Oh yes, we do need to set that too. This isn't Tutor-specific, so I wonder if there is a way we could set it as a default in all MFE repos.
In that case, couldn't we leave out PUBLIC_PATH, because the default value ( |
I'm with @kdmccormick on the reasons why we should avoid hard-coding yet another distribution into MFE repos. Which is to say, having Let's get back to the devstack, for a minute. One of the things it does is Correspondingly, there's nothing technical stopping us from adding similar functionality to Tutor. We don't need to go as far as |
Addendum: Kyle's use of the word "distribution" got me thinking: how does Debian solve this problem? For Meaning, I think I'm leaning towards |
PS: We can also use MFE domains as our "Plan A". In other words, we can start dropping |
OR... we can just start using the JS configuration mechanism proposed in that ADR, and just forget the dotenv files - a hairy subject that ADR by no coincidence also eschews. :) |
I think the easiest short-term "Plan B" solution would be writing out to Longer term I think moving away from |
Oh hey, I didn't even know +1 that the long term would be to move to |
With https://github.com/openedx/frontend-platform/pull/474/files nearing completion, I feel like the move from |
Update on this: As of brian-smith-tcril/tutor-mfe@ef070c9, I can now click on "Sign In" and the authn MFE loads properly. Once I try to actually log in, however, I get another CSRF error. with
Another thing of note is that when I delete the cookies and refresh the MFE page, no cookies are created. I do get cookies when loading the MFE by clicking the "Sign In" button:
I've tried changing the I will continue to investigate and post another update once I know more. |
To clarify: my plan here is to get the authn MFE working with tutor hardcoded to expect it at localhost, and the MFE using config from a checked-in |
Sounds good. I'm hoping |
I did a bit more digging into the error I mentioned here #25 and I'm at a loss for what to try next. The authn MFE appears to be correctly calling into It's not clear to me why authService.getCsrfTokenService().getCsrfToken("http://local.overhang.io:8000"); Even with it hardcoded, I'm getting the exact same CSRF error. Any advice on things to try or places I should be looking would be huge. I'm feeling pretty lost at the moment. |
Possible next steps:
|
Edit: This was specific to changes I had made earlier. The cookies work as expected on upstream tutor. |
Started down the caddy path, using https://caddy.community/t/setting-reverse-proxy-from-withing-docker-caddy-to-localhost-service/15369 as an example. One slight sticking point is:
I'm thinking this should be possible at the webpack level (ideally in the config in frontend-build). Once I've proved the concept I'll have a better sense of how to ensure the dev experience is as seamless as possible. |
Please see this PR, which is somewhat related: overhangio/tutor-mfe#137 |
Leaving a comment with the status of my discovery With the following changes (all pre overhangio/tutor-mfe#137):
When running
I know I tried a few cookie-related things (I think in edx-platform) but I can't find those changes at the moment. Hopefully this is helpful to anyone trying to add local |
Making a note here that @bradenmacdonald managed to run an MFE outside the container as part of his Vite PoC, but I don't see why we couldn't try to do the same thing (officially, I mean) with good-old webpack: |
Yes, since I figured out how to do it (with webpack), I run all my "dev" mode MFEs that way (on the host) - so much nicer to work with that way. |
Background
There are ~9 MFEs enabled by default in
tutor-mfe
now, and that number grows every release. In prod, all MFEs share one container, so it's not problem. In dev, though, we spin up one container per MFE. This consumes an absurd number of resources, and probably isn't necessary since MFEs seem to run fine on everyone's maching with a simplenpm clean-install && npm start
.Here's a good illustration of the problem:
https://discuss.openedx.org/t/after-installing-a-tutor-nightly-system-is-working-slow-which-container-can-i-stop-temporary/10681
In addition to resource consumption, making changes to MFE code in Docker containers through bind-mounts is unnecessarily complex. It'd be better if developers could just edit MFE code on their host and see the changes manifest immediately.
Tasks
Prototype an overhaul of tutor-mfe's dev mode that solves this problem, something along the lines of:
npm start
. Tutor should allow them to see the MFE that theynpm start
ed rather than the MFE pre-build MFE from the Docker container. The rest of the MFEs should continue to be served from the Docker container.Rationale
Goals:
Notes
Read @brian-smith-tcril 's comment here: openedx-unsupported/wg-developer-experience#154 (comment)
The text was updated successfully, but these errors were encountered: