-
-
Notifications
You must be signed in to change notification settings - Fork 324
chore: bump deps blocked by ESM #23249
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
Conversation
3eef0a0 to
0fcc2d3
Compare
|
✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
|
✅ Previously successful run of [Test] PR Suite Web e2e tests workflow has been found. |
| filePath: require.resolve(rootNodeModulesPath + '/uuid/dist/index.js'), | ||
| type: 'sourceFile', | ||
| }; | ||
| } |
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.
Otherwise you get
Android Bundling failed 47120ms suite-native/app/index.js (8598 modules)
Unable to resolve "uuid" from "suite-common/message-system/src/messageSystemUtils.ts"
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.
2c2b4f1 to
eb5abfb
Compare
9ff987e to
324e4c2
Compare
| "extends": "../../tsconfig.base.json", | ||
| "compilerOptions": { | ||
| "module": "commonjs", | ||
| "moduleResolution": "node", |
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.
deleting this effectively means changing to module: ESNext and moduleResolution: nodenext
(inheriting from tsconfig.base.json)
I don't know for what historical reason was it necessary to specify CJS module resolution for typescript, but now it isn't, because /src si bundled and /e2e is loaded via playwright typescript loader, which resolves ESM and CJS mostly quite well.
| '\\.(js|jsx|ts|tsx)$': ['babel-jest', babelConfig], | ||
| }, | ||
|
|
||
| transformIgnorePatterns: ['node_modules/?!(\\@noble)/'], |
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 couldn't have worked, because node_modules/\@noble does not exist,
so the ?! would never match
so the pattern would never match
so all node_modules would be ignored from babel transform = same as default.
Well, this means it was not needed 🙂
| modulePathIgnorePatterns: ['node_modules', '<rootDir>/lib', '<rootDir>/libDev'], | ||
| watchPathIgnorePatterns: ['<rootDir>/libDev', '<rootDir>/lib'], | ||
| testPathIgnorePatterns: ['<rootDir>/libDev/', '<rootDir>/lib/'], | ||
| transformIgnorePatterns: ['/node_modules/'], |
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.
superfluous; this is the default. Deleting this line so it uses the default in jest.base.config.js
suite-common/wallet-core/src/device/delegatedIdentityKey/getProofOfDelegatedIdentity.ts
Outdated
Show resolved
Hide resolved
324e4c2 to
56ae763
Compare
peter-sanderson
left a comment
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.
gigachad 🐶

Description
After bumping node to 24 in #23206, all our CJS–ESM pains have been washed away, magically solved in nodeJS nodejs/node#51977, so now everything will be 🌈 and 🦄 ❤️
Problems were mainly in these domains:
tsx, so we're loading TS files, which are written in ES, but the TS loader transpiles them on the fly to execute them in nodeJS in CJS modeAnd so I bumped all packages that were considered blocked:
electron-store(electron-main)@noble/curves(electron-main, playwright)uuid(playwright)chalk(electron-main, some scripts)QA instructions
desktop builds here
Suite Desktop: on Win, macOS, Linux, test that these settings are correctly persisted (
electron-store):electron-storeis not the primary storage of userdata, that's IDB which is both for Web & Desktop. Only some specific data that are related to Desktop app behavior in relation to OS are stored inelectron-storeSuite Desktop, Web, Android, iOS (
@noble/curves)getProofOfDelegatedIdentityhow to test??Also
uuid: Not much to test, it's a tiny utility hidden beneath. Testable is only desktop/web Message system manager (correctly generates random uuid).And some CI scripts which I have all tested:
And green CI means Playwright and Jest is OK 💚
Related issue
Resolve #22316
Resolve #14482
Notes
Actually, this nodeJS feature was even backported to 22.12, so we could have done it much sooner 🤦 But we were staying on 22.11 for the past year, and I only realized that now.. This is however highly relevant for electron-main, because electron-main environemnt is driven by electron, not by our repo. Electron ships with node 22.20 as of 39. That's why I electron store can be loaded in CJS mode, and again, it's something I could have done sooner, probably at electron 35?
Note that long-term, we might still want to bundle our Electron-main script as ESM, and run it as such. That would require abandoning webpack in electron-main, probably rollup instead. But it's not so hot now..
🔍🖥️ Suite desktop test results: View in Currents
🔍🖥️ Suite web test results: View in Currents
🔍🖥️ Suite native android test results: View in Currents