-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: migrations not being applied on 7.14.0 #8271
Conversation
…this is due to the fact that babel v7 do not compile the export default that it is ecma6 to commonJS as it did on babel5 since we ere using require on the migrations file, we needed to add a .default on each migration or use ecmascript file and use import instead, I followed the current pattern that we have on the app and used the import
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/28892b84-ebdf-4eda-b099-35260216c405 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8271 +/- ##
=======================================
Coverage 40.21% 40.21%
=======================================
Files 1235 1235
Lines 29883 29883
Branches 2862 2862
=======================================
Hits 12018 12018
Misses 17174 17174
Partials 691 691 ☔ View full report in Codecov by Sentry. |
Reverted changes to be able to merge it into main Here is the build version with artifacts for testing purposes on version 7.14.0 |
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
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.
Looks good to me. Thanks for making one more step toward using module import pattern everywhere!
Tested and working as expected, screen recording attached below:- RPReplay_Final1705421385.MP4 |
Confirming that this PR addresses the issue: http://recordit.co/oQoBimZE2Z |
Description
This pull request addresses an issue we were facing with migrations not being applied. The root cause was a change in how Babel v6 and v7 compiles ES6
export default
to CommonJS, which differs from its previous behavior in Babel v5.source: https://stackoverflow.com/questions/33704714/cant-require-default-export-value-in-babel-6-x
Previously, we were using
require
to load the migration files. However, due to the change in Babel's compilation, we would need to append.default
to each migration when usingrequire
, as Babel now compilesexport default
toexports.default
.To maintain consistency with the current pattern in our app and to avoid appending
.default
to each migration, I have switched from usingrequire
toimport
. This change aligns with our usage of ES6 syntax throughout the application and ensures that migrations are correctly applied.Related issues
Fixes:
Manual testing steps
-> Install 7.12.5
-> Import an account via SRP
-> Install 7.14.0
-> Vault recovery shouldn't happen
Screenshots/Recordings
This recording shows 7.14.0 after being updated from 7.12.5
IOS:
https://github.com/MetaMask/metamask-mobile/assets/46944231/a7618468-c816-4b3c-a900-19ab3c2c2d5a
Android:
https://github.com/MetaMask/metamask-mobile/assets/46944231/c97a03eb-153f-4bc3-9103-b2712f99e41f
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist