-
Notifications
You must be signed in to change notification settings - Fork 2
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
[api] adopt ESM throughout #3464
Conversation
83e8d87
to
6aa01c8
Compare
Removed vultr server and associated DNS entries |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
lib
andtarget
toesnext
module
andmoduleResolution
tonodenext
type: module
inpackage.json
, as well asexports
andengines
fields for clarity.js
file endings to all imports for ESM compliancepnpm test
) to use ESM (with--experimental-vm-modules
flag)ts-node-dev
(which is incompatible with ESM) withtsx
require(...)
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.json
changes 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-jest
to transform files for execution (which readstsconfig.test
, an extension oftsconfig
)pnpm dev
) relies ontsx
(which also reads thetsconfig
)