Site name that contains empty spaces is incorrectly displayed in browser tab #61
Comments
|
@regisb, Did you build the mfe image on a MAC/Zsh OS? |
|
Nope, a regular x64 Linux. |
|
I couldn’t reproduce, Can you provide more details;
Were you running which version of tutor, on which branch nutmeg or nightly? On dev or local mode and lastly which version/tag of the MFE
… On 3 Oct 2022, at 17:24, Régis Behmo ***@***.***> wrote:
Nope, a regular x64 Linux.
—
Reply to this email directly, view it on GitHub <#61 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD42PH4HG5RKEBWM642Y26TWBLUBPANCNFSM6AAAAAAQ3TDD7E>.
You are receiving this because you were mentioned.
|
|
You caught me! I'm usually the one asking for this kind of details :-p The issue is observed in dev mode, in particular in the account MFE: http://apps.local.overhang.io:1997/account Indeed, I expect that the issue will not occur in "local" mode because of an implementation quirk which I just realised: the values in mfe/build/mfe/env/ are parsed by bash in production: But in development they are parsed by docker-compose: The latter is the reason why we removed single quotes in the env files: #56 The fact that these files need to be parsed by two different tools (bash and docker-compose) is a source of confusion. I think that we should resolve that by making sure that these files are parsed only by bash. This way, we can add "..." quotes to the env files and stop bothering whether docker-compose will parse them correctly. I'll open a corresponding PR. |
|
That’s a good summary!, if I can suggest another possibility;
Why not bind production to .env and development to .env.development doing that would allow to change configuration without relying on env variables.
Fronted-build is already using this https://github.com/mrsteele/dotenv-webpack hence ref 1 <https://github.com/openedx/frontend-build/blob/95e8a8c24095f1bbe20d48c965cc7bfb75cbd464/config/webpack.dev.config.js#L20-L22>/dev and ref 2/prod <https://github.com/openedx/frontend-build/blob/95e8a8c24095f1bbe20d48c965cc7bfb75cbd464/config/webpack.prod.config.js#L194-L197> this web pack plugin I guess would transform variables from a file and then inject them in process.env module
|
Hmmm... that's possible, but:
Generally speaking, I wouldn't touch webpack tools with a ten foot pole, but if we can simplify the loading and overriding of settings by better leveraging webpack-env, then I'm all for it. @arbrandes what are your thoughts on this? |
With this change, we avoid the problem of having to escape env variables altogether. Previously, env variables were loaded both by bash (in development) and docker-compose (in production). This meant that we had to deal with escaping in both contexts, which was the source of trouble. We resolve this by loading env vars only with bash. Close #61.
I don't think so, the relevant code I found about this frontend-build, is that you can .env.private file which would override, .env.development:
Yes I tested this the other day, not through |
|
@regisb, @ghassanmas, just an ACK, for now: I'll take a look at the PR shortly. |
With this change, we avoid the problem of having to escape env variables altogether. Previously, env variables were loaded both by bash (in development) and docker-compose (in production). This meant that we had to deal with escaping in both contexts, which was the source of trouble. We resolve this by loading env vars only with bash. Close #61.
With this change, we avoid the problem of having to escape env variables altogether. Previously, env variables were loaded both by bash (in development) and docker-compose (in production). This meant that we had to deal with escaping in both contexts, which was the source of trouble. We resolve this by loading env vars only with bash. Close #61.
With this change, we avoid the problem of having to escape env variables altogether. Previously, env variables were loaded both by bash (in development) and docker-compose (in production). This meant that we had to deal with escaping in both contexts, which was the source of trouble. We resolve this by loading env vars only with bash. Close #61.
I'd rather leave those with each MFE's default values, and instead use the process environment to override them. This is more in line with how Docker does things. (Principle of Least Surprise, and all.) Which is what @regisb is doing in the PR which I'm about to approve. |
|
@ghassanmas, we can't remove env files as part of #69: there still needs a way for Tutor to override the default values non-dynamically per MFE, including the all-important config API URL. We could do it via patch to the Django settings files, but that only gets fetched after initialization. |
With this change, we avoid the problem of having to escape env variables altogether. Previously, env variables were loaded both by bash (in development) and docker-compose (in production). This meant that we had to deal with escaping in both contexts, which was the source of trouble. We resolve this by loading env vars only with bash. Close #61.
With this change, we avoid the problem of having to escape env variables altogether. Previously, env variables were loaded both by bash (in development) and docker-compose (in production). This meant that we had to deal with escaping in both contexts, which was the source of trouble. We resolve this by loading env vars only with bash. Close overhangio#61.
With this change, we avoid the problem of having to escape env variables altogether. Previously, env variables were loaded both by bash (in development) and docker-compose (in production). This meant that we had to deal with escaping in both contexts, which was the source of trouble. We resolve this by loading env vars only with bash. Close overhangio#61.
With this change, we avoid the problem of having to escape env variables altogether. Previously, env variables were loaded both by bash (in development) and docker-compose (in production). This meant that we had to deal with escaping in both contexts, which was the source of trouble. We resolve this by loading env vars only with bash. Close overhangio#61. (cherry picked from commit 9b1b08f)
This is how a site name that includes white spaces appears in my browser tab:
This bug is almost certainly caused by this recent change: #59
@ghassanmas I should have tested your change more thoroughly. Can you please suggest a fix?
The text was updated successfully, but these errors were encountered: