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

Chore/upgrade chai #65

Closed
wants to merge 9 commits into from
Closed

Chore/upgrade chai #65

wants to merge 9 commits into from

Conversation

mattdean-digicatapult
Copy link
Contributor

@mattdean-digicatapult mattdean-digicatapult commented Jan 5, 2024

Upgrade chai to v5 and switch to esm output. This PR superceeds #55

This was a pretty horrible upgrade but it all boilds down to chai (and in the future other dependencies) no longer supporting commonjs modules. The logic of this PR is therefore something like:

  1. chai no longer supports commonjs modules so we need to change the module type by adding "type": "module" to package.json
  2. we also have to change the tsconfig to output in native module format so use ESNext
  3. node when importing native esm modules needs to refer to an exact file path (no directories or names without extensions). Because tsc won't mangle import names this necessitates adding .js extensions to all local imports (yes typescript understands what this means)
  4. When executing tests with mocha or using nodemon we need to now tell it how to load modules. Usually this involves adding --loader ts-node/esm to any node invocations but this now generates a warning in node asking us to switch to the vastly more horrible --import 'data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("ts-node/esm", pathToFileURL("./"));'. Reading arround this seems to be where node is heading so 🤦‍♂️
  5. Because we're using native node import rather than require at runtime we can no longer load json without generating an experimental warning so I've changed that to read the swagger file manually

@mattdean-digicatapult mattdean-digicatapult marked this pull request as ready for review January 5, 2024 12:50
@mattdean-digicatapult mattdean-digicatapult requested a review from a team as a code owner January 5, 2024 12:50
n3op2
n3op2 previously approved these changes Jan 5, 2024
Copy link
Contributor

@n3op2 n3op2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm... as far as I remember this was a bit weird that we need to import from build rather than .ts and specify index file

@mattdean-digicatapult
Copy link
Contributor Author

hm... as far as I remember this was a bit weird that we need to import from build rather than .ts and specify index file

Yes I need to have another look at the knex commands. I think these are the only places I now have issues

@mattdean-digicatapult
Copy link
Contributor Author

knex/knex#4793 does not look good

@mattdean-digicatapult
Copy link
Contributor Author

I've managed to fix this using:

{
  "db:cmd": "node --import='data:text/javascript,import { register } from \"node:module\"; import { pathToFileURL } from \"node:url\"; register(\"ts-node/esm\", pathToFileURL(\"./\"));' ./node_modules/.bin/knex",
  "db:migrate": "npm run db:cmd -- migrate:latest --knexfile src/lib/db/knexfile.ts",
  "db:rollback": "npm run db:cmd -- migrate:rollback --knexfile src/lib/db/knexfile.ts"
}

@mattdean-digicatapult
Copy link
Contributor Author

Broadly speaking I dislike all the horribleness especially needing to wrap all node commands in essentially:

--import='data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("ts-node/esm", pathToFileURL("./"));'

This is basically caused by ts-node being massively behind the curve TypeStrong/ts-node#1997. As they don't seem to be responding it might be worth looking for a ts-node alternative and the two best options I've seen are:

I'm not sure I want to do this switch right now, but we can consider it

Copy link
Contributor

@dc-andysign dc-andysign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

package.json Show resolved Hide resolved
@mattdean-digicatapult
Copy link
Contributor Author

closing in favour of #66

@mattdean-digicatapult mattdean-digicatapult deleted the chore/upgrade-chai branch January 5, 2024 16:48
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.

None yet

3 participants