Conversation
83e8d87 to
6aa01c8
Compare
|
Removed vultr server and associated DNS entries |
DafyddLlyr
left a comment
There was a problem hiding this comment.
Thanks for bearing with me whilst I got through this one! A bit more complexity than initially anticipated, but certainly a worthwhile change to have made for a whole host of reasons.
Looking great and working as expected. A few comments which can be fixed here or as follow up PRs - very much your call.
39bef8a to
e5ff9fa
Compare
Thanks! Definitely picked up a lot of nuances about TypeScript etc while moving through this - have tried to list any particularly helpful resources in the PR description for posterity. I've addressed all your comments and rebased onto main, so I think this should be good to go? Maybe just have a quick look at my changes (commit e5ff9fa) ✨ |
There was a problem hiding this comment.
Latest commit looks good to me! Lots of good stuff here, thanks for working through it all & carefully noting so many docs/resources 🎉
Let's get a prod deploy through first this morning (queued up here #3503), then this can merge today & hang out on staging for a few days into early next week just in case anything comes up 🙂
… ESM modules in package.json, add 'exports' and 'engines' fields
… enabled, bump ts-jest
for tests), improve lodash imports
e5ff9fa to
da25ab5
Compare
This PR does the following:
libandtargettoesnextmoduleandmoduleResolutiontonodenexttype: moduleinpackage.json, as well asexportsandenginesfields for clarity.jsfile endings to all imports for ESM compliancepnpm test) to use ESM (with--experimental-vm-modulesflag)ts-node-dev(which is incompatible with ESM) withtsxrequire(...)with top-lineimport * as x from ...statementsThe motivation here is to enable top-level await statements, which is required for the Microsoft SSO work (see #3453).
Will be best reviewed commit-by-commit. 17125bf is small, handling the initial
tsconfig.jsonchanges to force adoption of ESM throughout, while 89c2ee6 is huge but fairly straightforward.5cae484 is perhaps the meatiest, and also required the changes in planx-core#465 to get tests running. Note that planx-core#464 was also prompted by this work, enabling the fixes to imports in b29fab9.
Resources
Still to investigate (possibly after this PR is merged)
api-add-microsoft-sso-strategy-2?require(...)statements withawait import(...)where they occur (what I did in 3f221a6, but had to further adapt in 6aa01c8)? And if not is ts-jest still just configured incorrectly/being buggy, are the files being interpreted as cjs still, or is it something else?Should I be able to import files as.ts, rather than.js? As I understand, this would only be possible if we weren't usins TS to 'emit' JS. One option to explore could be to usemoduleResolution: "bundler"instead of "nodenext".Note that although some changes may survive type checking, and even build, this doesn't necessarily imply that the tests will check out. The code needs to be solid from various angles:
tsc(which readstsconfig)ts-jestto transform files for execution (which readstsconfig.test, an extension oftsconfig)pnpm dev) relies ontsx(which also reads thetsconfig)