Skip to content

[api] adopt ESM throughout#3464

Merged
freemvmt merged 8 commits intomainfrom
api-bump-esnext
Aug 9, 2024
Merged

[api] adopt ESM throughout#3464
freemvmt merged 8 commits intomainfrom
api-bump-esnext

Conversation

@freemvmt
Copy link
Copy Markdown
Member

@freemvmt freemvmt commented Jul 26, 2024

This PR does the following:

  • in tsconfig, bumps lib and target to esnext
  • in tsconfig, bumps module and moduleResolution to nodenext
  • adds type: module in package.json, as well as exports and engines fields for clarity
  • adds .js file endings to all imports for ESM compliance
  • makes significant changes to the Jest config in order to play nicely with ESM
  • similarly adapts the test scripts (e.g. pnpm test) to use ESM (with --experimental-vm-modules flag)
  • replaces ts-node-dev (which is incompatible with ESM) with tsx
  • replaces any instances of require(...) with top-line import * as x from ... statements

The 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)

  • Check the codebase can now handle a top-level await, as I intend to use in api-add-microsoft-sso-strategy-2?
  • If so, is there a good reason why I can't directly replace require(...) statements with await 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 use moduleResolution: "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:

  • Type checking, as well as building the actual application, rely directly on tsc (which reads tsconfig)
  • Tests (Jest) relies on ts-jest to transform files for execution (which reads tsconfig.test, an extension of tsconfig)
  • The dev environment (pnpm dev) relies on tsx (which also reads the tsconfig)

@freemvmt freemvmt changed the title [api] adopt ESM throughout [WIP][api] adopt ESM throughout Jul 26, 2024
@freemvmt freemvmt changed the title [WIP][api] adopt ESM throughout [api] adopt ESM throughout Jul 27, 2024
@freemvmt freemvmt changed the title [api] adopt ESM throughout [WIP][api] adopt ESM throughout Jul 27, 2024
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 1, 2024

Removed vultr server and associated DNS entries

@freemvmt freemvmt requested a review from a team August 1, 2024 17:39
@freemvmt freemvmt changed the title [WIP][api] adopt ESM throughout [api] adopt ESM throughout Aug 1, 2024
Copy link
Copy Markdown
Contributor

@DafyddLlyr DafyddLlyr left a 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.

Comment thread api.planx.uk/jest.config.ts
Comment thread api.planx.uk/modules/gis/service/index.js Outdated
Comment thread api.planx.uk/modules/auth/service.ts Outdated
Comment thread api.planx.uk/modules/file/service/uploadFile.ts
Comment thread api.planx.uk/helpers.ts Outdated
Comment thread api.planx.uk/package.json Outdated
@freemvmt freemvmt force-pushed the api-bump-esnext branch 2 times, most recently from 39bef8a to e5ff9fa Compare August 9, 2024 03:14
@freemvmt
Copy link
Copy Markdown
Member Author

freemvmt commented Aug 9, 2024

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.

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) ✨

Copy link
Copy Markdown
Member

@jessicamcinchak jessicamcinchak left a 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 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants