Site name that contains empty spaces is incorrectly displayed in browser tab
Closed this issue · 11 comments
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?
@regisb, Did you build the mfe image on a MAC/Zsh OS?
Nope, a regular x64 Linux.
You caught me! I'm usually the one asking for this kind of details :-p
$ tutor --version
tutor, version 14.0.5
$ tutor plugins list | grep mfe
mfe 14.0.1
$ tutor dev start -d
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:
RUN bash -c "set -a && source /openedx/env/production && source /openedx/env/production.override && npm run build"
But in development they are parsed by docker-compose:
env_files:
- ../plugins/mfe/build/mfe/env/production
- ../plugins/mfe/build/mfe/env/development
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.
Why not bind production to .env and development to .env.development doing that would allow to change configuration without relying on env variables.
Hmmm... that's possible, but:
- In development, are values from .env also loaded? If yes, in which order? If not then we need to list all variables in .env.development: currently this is not the case, because development values only override production values.
- Do environment variables override values in .env/.env.development? We need to make it possible for some MFEs to override certain values, as we currently do with
*_MFE_APP["env"]["production"].
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?
1.In development, are values from .env also loaded? If yes, in which order? If not then we need to list all variables in .env.development: currently this is not the case, because development values only override production values.
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:
ref code
ref doc
- Do environment variables override values in .env/.env.development? We need to make it possible for some MFEs to override certain values, as we currently do with *_MFE_APP["env"]["production"].
Yes I tested this the other day, not through *_MFE_APP["env"]["production"] per say, I injected env varaiable before the bash command, i.e. NAME=VALUE npm run build that ovrirde the value in .env file.
@regisb, @ghassanmas, just an ACK, for now: I'll take a look at the PR shortly.
Why not bind production to .env and development to .env.development
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.