-
Notifications
You must be signed in to change notification settings - Fork 64
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
docs: ADR that proposes a common stylesheet for all MFEs #351
Conversation
Thanks for the pull request, @xitij2000! 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. |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #351 +/- ##
=======================================
Coverage 83.05% 83.05%
=======================================
Files 40 40
Lines 1062 1062
Branches 195 195
=======================================
Hits 882 882
Misses 168 168
Partials 12 12 ☔ View full report in Codecov by Sentry. |
Demo instance using a similar mechanism to load a theme: https://apps.bb4693.sandbox.opencraft.hosting/discussion/course-v1:edX+DemoX+Demo_Course/posts |
The mechanism for loading this common stylesheet will need to be different from | ||
the mechanism for loading the MFE stylesheet. A JavaScript theme loader will | ||
contain the location of the SCSS file and will add this stylesheet to the | ||
document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exact mechanism of this is currently not in this ADR but will be the subject of another ADR if this is accepted.
The prototype presented here uses Webpack's module federation, however for the purpose of just a single stylesheet, this may be an overkill. It might make sense to create a specific loader for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, one of the big benefits of MFEs is the ability to very quickly iterate on an individual MFE and deploy changes. Can you talk about that in the context of this ADR? e.g. does this ADR require that all MFEs are using the same version of Paragon, or is it still possible for one MFE to be using a newer version of Paragon (and newer common CSS) while other MFEs haven't yet been updated (and so use a different common CSS file?). And in that case, if multiple MFEs can use different versions of the common CSS file, are theme updates still faster?
file names change, ensuring that a file is cached as long as it's used but not | ||
loaded any more once the file contents change. | ||
|
||
Since the common theme needs to have a fixed name, we can't rely on changing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This confused me at first. I think what you're saying is "Without using a JavaScript based loader on the client side, and without support for pre-rendering our MFEs on the server side, we would need to use a fixed URL/filename for the common CSS file, which would cause problems with caching that file."
Because in your actual ADR, the common theme does not need to have a fixed name ("now it can be a file with
the hash in its name").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I might not have been very clear about this.
Currently, the file names generated for all the static content have a hash in the name, which allows indefinite caching. However, if you need to have the SCSS in a common location known to all MFEs in advance, this file name cannot change with each update, otherwise you'd need to redeploy all MFEs anyway. So you need a fixed static URL.
However, if the fixed static URL is the stylesheet itself, it will be a large file that cannot be cached, otherwise updating the stylesheet will not work as well and will require the cache to expire before updates show up.
So the idea is to have a small simple JavaScript loader that is never cached, and contains the location of this SCSS file which does have a hash in its name. This way only a minimum size is uncached and the rest can be cached.
I've experimented with this a bit, and in my experience things work pretty well even with different versions of Paragon across MFEs. That said, there can be sources if issues, one being the following situation: There is a bug in a Paragon component that requires a JavaScript and CSS fix. The version of Paragon including the bug fix is updated for a particular MFE, but not for the common theme. So the JavaScript for the Paragon bug fix does get deployed, but not the CSS code. I think it will be important to keep Paragon in the common theme pinned to the latest version used by all MFEs, and to redeploy the common theme each time an MFE is updated and redeployed if it includes an update to Paragon. |
Hmm, that does seem like a significant drawback. Could it make sense to include the JS and CSS together in the common theme code, so that they're always in sync at least? Also: It seems that the main benefits of this ADR accrue when someone is actively editing or updating themes. But I imagine for each given Open edX instance, that will only occur occasionally (e.g. when first setting the instance up), and 99% of the time it will be operated and maintained without anyone making changes to theming. (Or is that a wrong assumption?). If so, would it make sense to have a kind of "theming mode" where this ADR's functionality is enabled, but otherwise leave it off for normal operations and faster deployment of MFE updates? Or alternatively, would it make sense to use the approach specified in this ADR for instances using named release versions (favoring rapid theming, and where MFEs are probably using the same Paragon version anyways), but use the current method for instances that are deployed continuously (favoring flexibility, accuracy, and rapid iteration for each individual MFE)? |
This is possible if we just use module federation for the theme, i.e. have a Another simpler solution is to switch to using CSS variables in Paragon. In this case Paragon no long needs to be built with the theme but can be built and loaded independently by each MFE. However, this reduces the benefits of consolidating the theme in the first place. I think we can also simplify this with build automation. i.e. part of the MFE deployment pipeline can be to trigger a rebuild of the common theme. This common theme can have versions pinned such that it always takes the latest version of the branding package, and can have hooks that update the common theme to always have the latest version of Paragon that is used by any MFE (not the latest released version, the latest used version).
That is not entirely wrong if we take this ADR in isolation, but I also don't see this as a stand-alone thing. For example, in combination with the configuration API ADR, you can have a multi-tenent setup, each site having a separate theme.
That is mentioned in passing in this ADR, i.e. that the existing mechanism will continue to be supported. If you don't provide a config variable for the common theme location, then it will build and use the theme the way it does now.
I had another idea, but it needed more investigation, which was to have the Paragon version included in the common theme code. Then the MFE can decide to load the common theme or the theme it comes with. If the MFE uses a newer version of Paragon than is provided by the common theme, then it can load the CSS it comes with. Both of these will be built with the latest variables so should be intact. The next time the main theme is rebuilt it will have a newer version and as such it will once again be used. That said, I think all this will need separate investigation and a separate ADR. I hoped to keep this initial ADR simple and focussed on one thing. |
Cool. Sorry if I muddied the waters :p |
I am very grateful for the feedback! I think it will help clarify the ADR a lot more, and flesh out more clearly the caveats in this approach. |
@xitij2000 Thank you for your contribution. Please let me know once this is ready for a review. |
a0d2e5c
to
48146c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the proposal of this ADR. As always, the detail of the implementation will be the critical factor but the proposed road looks good to me.
directly. This means that if different versions of Paragon are used for an MFE | ||
vs the common theme, then there is a chance that an incompatible version of the | ||
CSS could be loaded for the MFE. This will make it all the more important to | ||
ensure that version of Paragon used across all MFEs is compatible. This will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am imagining that making paragon compatible only happens one time. Before a mfe uses this compatible version of paragon then it is business as usual. After, they are loading the common css and that is it.
Do you foresee that there could be other moments where compatibility is broken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understood what you meant by "making paragon compatible only happens one time".
I think the issue I'm looking at is as follows:
- A bug is encountered in a Paragon component. For example if a label/text for some component is longer than a certain threshold, it will break the UI.
- The fix for this involves some React code and a change in SCSS.
- The common compiled theme is built on an older version of Paragon that doesn't include this fix.
- An MFE is updated to include a newer version of Paragon.
- The MFE is now using React code, but it's supporting CSS code isn't available in the theme.
This would need the common theme to be rebuilt as well, using the latest Paragon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now. I thought you were the initial compatibility change, but this issue will come about constantly.
By continuing to support the existing theming mechanism, the current mechanism | ||
for theming can continue working for those who need it. | ||
|
||
Since there is a JavaScript-based loader involved, there is potential for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that getting it right for the loader is the gist of this proposal.
Would it be possible that there is more than a loader written and which loader is used is also a configuration? I am looking at this from the lens of multitenancy probably requiring a more complex/slower loader which not everyone should pay the penalty for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this proposal I imagine that the loader can be as simple as:
const urlOfThemeCSS= '...';
addCSSStyleSheet(urlOfThemeCSS);
Where the function will essentially create a link
element to load the stylesheet. In addition, it can have a simple API to signal to the app when the theme has loaded.
I think a good path for multi-tenancy would be to combine this with the great work being done on runtime configuration: openedx/wg-frontend#103
Since the loader path is a config variable, and the runtime configuration can provider URLs that vary based on the site, it would allow pointing to different themes. The loader itself can just remain simple, if that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you are saying then is that the loader sis so simple that there will be no need for more than one. If the loader ends up being something so flexible, then the config variable will take care of everything.
I was trying to avoid a situation where there is already logic baked into the loader that makes it imposible to extend via configuration. It does not look like that is the case though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loader ideally needs to be simple, since the idea is that it won't be cached. So the smaller it is the better.
Right now the scss name is baked into the index.html file. If the stylesheet needs to load dynamically we can't do that. So we need some way to dynamically determine the name of this stylesheet file. One mechanism is to have a more complex API that returns the stylesheet location. An alternative is to have a small javascript file (the loader) that has the location of the stylesheet in it. Whenever the stylesheet location changes this file will be updated, and it will always be reloaded, so we get the latest stylesheet.
So making the loader heavy would be detrimental to the loading speed, though there is probably nothing stopping people from doing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in the html we would have:
<!DOCTYPE html>
<html>
<head>
<script type="text/javascript" src="loaderfile_with_no_cache.js"></script>
<script type="text/javascript" src="long_react_code.123HASH134.js"></script>
</head>
And if someone wants to override the loader, they can go and configure the thing to load the file from anywhere else?
e.g:
<!DOCTYPE html>
<html>
<head>
<script type="text/javascript" src="different/path/custom_loaderfile_with_no_cache.js"></script>
<script type="text/javascript" src="long_react_code.123HASH134.js"></script>
</head>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felipemontoya I think we'd load the loader itself dynamically, and just append the current timestamp as a query parameter to break the cache.
However, the location of this loader would be a config variable provided during build, or with the runtime configuration mechanism dynamically during runtime.
@import "~@edx/paragon/scss/core/core"; | ||
@import "~@edx/brand/paragon/overrides"; | ||
|
||
These four lines, import the Paragon and bootstrap code and apply |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @xitij2000, I think this is a good idea with the actual status of the theming.
I was wondering about the header, footer, and cookie banner styles, that should be updated to use CSS variables, update the version in the MFE and be part of the MFE stylesheet or could be part of the theme stylesheet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the header, footer, cookie banner and other elements shoudl also over time be updated to use CSS variables. They can then be independent stylesheets that are loaded for those.
So we'd have:
- Base theme stylesheet (loaded from a common source)
- Component stylesheets (header, footer, banner etc. separate for each MFE)
- MFE stylesheet (separate for each MFE).
I think this will depend a lot on the mechanism being used for this. I was experimenting on an approach using a monorepo for all learner MFEs using npm workspaces. i.e. each MFE is a package inside a larger global repo, with MFEs loading each other using module federation. This, if set up properly would allow using each MFE individually, but also via the container SPA. If the mechanism that's finally settled on is similar, then it might still be possible to have the theme installed and built for the container SPA but still use and reference it in individual MFEs. For other approaches, I agree, the approach for individual MFEs in local development can remain much the same, i.e. build a local copy of the branding package point the individual MFE to it. |
Hi @xitij2000 and @arbrandes just checking in on this :) |
@mphilbrick211, @xitij2000 might have more to say, but I believe this is in the backburner, pending further development on openedx/open-edx-proposals#410 and openedx/paragon#1546. Both will have a significant impact on how theming will be done in the future. |
[inform/update] @viktorrusakov and I put together a prototype of what we imagine could work to dynamically load common stylesheet(s) generated by Paragon's design tokens into MFEs might look like: #440 The idea being that Paragon is going to compile its own CSS now with the recommendation to host it externally on a CDN (technically, even an open source CDN like jsdelivr or unpkg would work here). Would love any feedback on the proposed approach in the above PR. tl;dr;
|
@arbrandes @adamstankiewicz just checking in on this to see if there are any updates. |
I think the new PR linked by @adamstankiewicz above makes it possible to implement this in practice, however I'm still not up to date on the modular MFE discovery and whether it resolves the issue of different MFEs using different version of Paragon. With that PR some of the reasons for using a JavaScript loader are no longer applicable. Since the variables.css file is likely to be much smaller, it might be OK for it to have a short caching period, so it can be quickly updated. Or, with each variable a random query param can be added to the config API response so that the URL is forcefully refreshed. |
That's one of the first goals to solve, but we don't have the details, yet. But it's heading towards a world where MFEs will be encouraged to use Paragon from the containing SPA, but we won't be able to force them to do it. (This entails Paragon being very careful about backwards compatibility, but we knew that from the get-go.) Given that not using a global Paragon will likely exclude the MFE from features such as the one proposed in this PR, I expect all of them - or at least, all of the ones where this feature would be important - to follow the same pattern. |
Do you think it makes sense to update this discovery to take into account the recent developments with style dictionary, and @adamstankiewicz's PR: #440, given that the goal of modular MFEs is already to align all MFEs to the same version of paragon? I'm planning to go through the modular MFE discovery soon to familiarise myself with it as well. |
We're very much taking it into account, yes! :) |
Hi @arbrandes and @xitij2000 - just following up to see if there's an update here? |
I'm going to spend some time updating this PR this week so it aligns better with the latest developments. |
48146c1
to
95837f5
Compare
I've updated the PR to more closely match the current implementation. |
Just checking in on this @xitij2000 :) |
I had updated it to match the new implementation as of that comment. If there have been further changes I will allocate some time next week to update it again. |
Hi @xitij2000 and @arbrandes! Just checking in on this :) |
Adds an ADR that proposes creating a single common stylesheet that all MFEs load from a central place.
95837f5
to
f1b7440
Compare
I've updated the ADR to match changes, however I think it might be better to wait till that work is over, and have @adamstankiewicz work on this doc since I'm no longer fully up to date on the nuances of the recent changes. |
Hi @xitij2000 @adamstankiewicz - checking back in on this :) |
Sorry I haven't had much time to check into this due to time off recently. I don't think the PR this is tracking has changed since I last updated this ADR. |
Hey @xitij2000 , What is the current status of this PR, is it ready to review and merge? |
I am going to close this PR for now and look into it when the paragon PRs are closer to final. |
@xitij2000 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. |
1 similar comment
@xitij2000 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:
Adds an ADR that proposes creating a single common stylesheet that all MFEs load from a central place.
Merge checklist:
frontend-platform
. This can be done by runningnpm start
and opening http://localhost:8080.module.config.js
file infrontend-build
.fix
,feat
) and is appropriate for your code change. Consider whether your code is a breaking change, and modify your commit accordingly.Post merge: