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

Upgrade TypeScript & other deps #588

Merged
merged 6 commits into from
May 13, 2020
Merged

Conversation

LachlanStuart
Copy link
Contributor

@LachlanStuart LachlanStuart commented May 13, 2020

Webapp dependency updates:

  • TypeScript 3.6.5 -> 3.9.2
  • ElementUI 2.12.0 -> 2.13.1 (needed a TypeScript fix from 2.13.0)

Graphql dependency updates:

  • TypeScript 3.6.2 -> 3.9.2

Graphql dependency updates to resolve security alerts:

  • @sentry/node 5.4.0 -> 5.15.5
  • knex 0.19.5 -> 0.20.15 (cannot go to 0.21 due to node 8 EOL 😞 )
    • Note that knex is now only used for tests and the unused DB versions of the image storage
  • jest 24.9.0 -> 25.5.4 (cannot go to 26 due to node 8 EOL 😞 )
  • ts-jest 23.1.3 -> 25.5.0
  • nodemon 1.19.1 -> uninstalled, as we've been using ts-node-dev instead

Due to changes in jest behavior, globalSetup/globalTeardown had to be converted to TS to prevent a type-checking error from double-transpilation. There's a Jest bug causing TypeORM migrations to not be transformed, so I added a workaround that imports everything as usual (via the ts-jest transform), but registers ts-node just before TypeORM runs the migrations, so that they're transformed via ts-node.

Also, I wasn't able to find the root cause, but after these changes it was much more common for the first test to timeout during setupTestFramework's beforeAll or beforeEach, causing entire test suites to randomly fail. I strongly suspect that beforeAll wasn't previously included in the timeouts, as it would regularly take 9-11s to run this function in CircleCI. I bumped the testTimeout from 5s to 15s to resolve this.

Noteworthy changelogs:

TypeScript 3.6.5 -> 3.9.2
ElementUI 2.12.0 -> 2.13.1 (needed a TypeScript fix from 2.13.0)
TypeScript 3.6.2 -> 3.9.2
@sentry/node 5.4.0 -> 5.15.5
knex 0.19.5 -> 0.20.15 (cannot go to 0.21 due to node 8 EOL)
jest 24.9.0 -> 25.5.4 (cannot go to 26 due to node 8 EOL)
ts-jest 23.1.3 -> 25.5.0
nodemon 1.19.1 -> 2.0.3
Due to changes in jest behavior (running globalSetup in a worker process), globalSetup/globalTeardown had to be converted to TS, and a workaround for transpiling migrations had to be added
@LachlanStuart LachlanStuart force-pushed the feature/upgrade-typescript branch from 839a8b1 to 4c62bfe Compare May 13, 2020 14:30
@LachlanStuart LachlanStuart marked this pull request as ready for review May 13, 2020 14:53

module.exports = async () => {
export = async () => {
Copy link
Member

Choose a reason for hiding this comment

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

this is new syntax for me, quickly looked up and found some old discussion here: microsoft/TypeScript#7185

is this for commonJS compat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. module.exports = and export = are functionally equivalent. Both would be imported with globalSetup = require(...) / import * as globalSetup from '...'. In fact, TypeScript allows both forms of export, I just figured it would be better to match syntax with the rest of the file.

I thought it was an ES Modules thing, not a TypeScript thing, but I could be mistaken...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MDN and Node's docs don't have any mention of it, so I guess it is a TypeScript thing after all.

@@ -42,7 +42,7 @@
"d3": "^5.11.0",
"date-fns": "^1.29.0",
"dom-to-image-google-font-issue": "^2.6.2",
"element-ui": "^2.12.0",
"element-ui": "^2.13.1",
Copy link
Member

Choose a reason for hiding this comment

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

I'm still bearing the static CSS in mind, might need to change it sooner now. We might be missing a few fixes from this version, but it would take a lot of fixes to fix Element! 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually more concerned that ElementUI's updates will include intentional style changes that will break our layout, than concerned that our old static CSS will stop being compatible.

Fortunately not much has changed: ElemeFE/element@v2.12.0...v2.13.1

The only component with style changes that I'm concerned about is ElTable, as those tweaks could potentially change our AnnotationsTable layout a bit.

@LachlanStuart LachlanStuart merged commit 419325f into master May 13, 2020
@LachlanStuart LachlanStuart deleted the feature/upgrade-typescript branch May 13, 2020 15: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.

2 participants