-
Notifications
You must be signed in to change notification settings - Fork 23
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
Upgrade MLP library dependency version #114
Conversation
It makes sense that
It's probably safer to add the singleton config in the host app (Turing) too. |
ui/package.json
Outdated
@@ -5,7 +5,7 @@ | |||
"dependencies": { | |||
"@elastic/datemath": "5.0.3", | |||
"@elastic/eui": "32.3.0", | |||
"@gojek/mlp-ui": "1.4.10", | |||
"@gojek/mlp-ui": "^1.4.10", |
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.
@romanwozniak do you have any concerns with using a version range for the MLP dependency?
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.
Using a version range means that we should pay attention to the backward compatibility of the library, which we aren't doing right now. Can we, maybe, start with the range over patch versions (i.e. 1.4.x
or ~1.4.10
) as they are expected to be compatible?
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.
Okay. That sounds reasonable. Agree with using range over patch versions.
@terryyylim could you also update the PR description with any reference articles / docs you had used to understand the dependency sharing better? Will be useful for future reference. Thanks. |
resolved "https://registry.yarnpkg.com/@gojek/mlp-ui/-/mlp-ui-1.4.10.tgz#11af092be8d1d9779ce4dbc04dab8c181646d19b" | ||
integrity sha512-wJ/Bp6VWDHwRiruuXRc5p60izLV7ggf9JNI1suut/tarToxtaW1AC6GLTafqetwIRyURuTDC5QPNKhgSG0Dd2w== | ||
"@gojek/mlp-ui@^1.4.10": | ||
version "1.4.15" |
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.
Currently, 1.4.x resolves to 1.4.15
, which is also the version used by the internal experiment engine. If you haven't already tested the following scenarios, could you help verify them?
- Host app (Turing) uses a lower version of the dependency. We can force Turing to download
1.4.10
using resolutions while keeping~1.4.10
in the "dependencies" spec. - Child app (Exp engine) uses a lower version of the dependency than the host app
In both scenarios, 1.4.15
should be picked (can we confirmed using the exp details view in the child app).
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 believe it might be because we’re dynamically importing the remote dependencies (from Internal Experiment Engine - index.js
, bootstrap.js
), which are not known at Turing app start (because its async and not static), and as such, Module Federation is unable to draw the remote dependency versions into consideration (i.e ~1.4.10
) during the initialisation phase of Turing app and hence, result to using 1.4.10
.
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 believe it might be because we’re dynamically importing the remote dependencies
You're right about this and I think dynamic import is the key idea here. The article you shared earlier is useful in understanding this further: https://www.angulararchitects.io/en/aktuelles/getting-out-of-version-mismatch-hell-with-module-federation/
"While in the case of classic (static) Module Federation, both applications would agree upon using version 1.0.1 during the initialization phase, here in the case of dynamic module federation, the shell does not even know of the micro frontend in this phase. Hence, it can only choose for its own version"
The other key idea is singleton:
"Unfortunately, when the dynamic micro frontend is loaded, module federation does not find an already loaded version compatible with 1.0.1. Hence, the micro frontend falls back to its own version 1.0.1."
Now, for as long as Turing and the EE (Experiment Engine) were using the same version of @gojek/mlp-ui
, I believe the "already loaded" (Turing's) MLP dependency could be reused by the EE, effectively having only one copy of the library. So, we had no issue. When the EE bumped the dependency version up, we were having 2 separate copies which, because of whatever "global states" are held by the MLP UI components, wasn't working anymore and leading to the crash.
So, it makes sense that a singleton
is required. Additionally, with the whole idea of dynamic loading, we can conclude that it is Turing's dependency version that matters and NOT the EE's, for the remote module scenario (i.e., when the EE component is accessed from Turing). So your fix is working now, not because ~
is added to the MLP version dependency in Turing (and it is compatible with that of the EE), but because it is resolved to 1.4.15
today. Meaning, in the future, if the EE starts pointing to 1.4.16
of MLP, Turing will have to be re-deployed as well to make it use the new version of MLP.
Some thoughts on this:
- I think changing to static import wont help either - If EE is independently being upgraded (a new deployment where MLP version is increased), I doubt that Turing will recognise this and start using the new MLP version. It's still singleton and Turing wasn't re-deployed.
- Having to redeploy Turing when the EE wants to use a new version of MLP is not ideal. But I think it's not very different from our set up today and may be, in the future, MLP can be consumed remotely in all our apps which would fix this.
This is how I understand it now. If you have other thoughts, do share. If not, could you please help update the MR description? And I'd also then suggest putting the dependency version as 1.4.15
in Turing and go back to being explicit and leave a note in our EE's craco config about 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.
with the whole idea of dynamic loading, we can conclude that it is Turing's dependency version that matters and NOT the EE's
Yeap, this is my understanding as well. Have updated the PR.
9bfb71a
to
72d2e12
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.
LGTM. Thanks for the investigations, @terryyylim ! The dependency sharing's behaviour is a lot clearer to me now. 🙏🏽
With the introduction of Module Federation previously, we'd ideally utilise
singleton
property for dependencies which hold states (such as@gojek/mlp-ui
). This would prevent the parent application from crashing even different "global states" are used by it's child applications.In
v1.4.15
for@gojek/mlp-ui
, BigInt conversion support was added which is required by our internal experiment engine's (EE) MFE. Due to the nature ofdynamic import
, Module Federation was unable to find an already loaded version compatible for using by the EE's MFE, and hence, the dependency version that mattered was the parent app's dependency version (i.e Turing), and not EE's MFE dependency.Hence, this PR upgrades the dependency version and sets
singleton
property in Module Federation for shared library@gojek/mlp-ui
.Test Scenarios
@gojek/mlp-ui
library on respective apps