Skip to content
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

Merged
merged 8 commits into from
Aug 9, 2024
Merged

[api] adopt ESM throughout #3464

merged 8 commits into from
Aug 9, 2024

Conversation

freemvmt
Copy link
Contributor

@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
Copy link

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
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.

api.planx.uk/jest.config.ts Outdated Show resolved Hide resolved
api.planx.uk/modules/gis/service/index.js Outdated Show resolved Hide resolved
api.planx.uk/modules/auth/service.ts Outdated Show resolved Hide resolved
api.planx.uk/helpers.ts Outdated Show resolved Hide resolved
api.planx.uk/package.json Outdated Show resolved Hide resolved
@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
Contributor 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
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 🙂

@freemvmt freemvmt merged commit 16d71e8 into main Aug 9, 2024
12 checks passed
@freemvmt freemvmt deleted the api-bump-esnext branch August 9, 2024 12:47
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