-
Notifications
You must be signed in to change notification settings - Fork 106
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 to v5 #1053
Upgrade TypeScript to v5 #1053
Conversation
See notice deprecation: https://www.npmjs.com/package/tslint
Incorporates various changes to package dependencies, Rollup and TS config in order to bump up the main TypeScript version to 5.4.4
Fixes `super` lookups that weren’t able to be resolved due to TypeScript not recognising the context in arrow functions declared as properties on the class or constructor. See: https://github.com/basarat/typescript-book/blob/master/docs/arrow-functions.md#tip-arrow-functions-and-inheritance
Might be a good idea to generalise this record type for reuse throughout this module rather than declaring everything repeatedly inline.
The global `jasmine` API has been removed from Jest with the new test runner `jest-circus` introduced in v27–v29. While possible to import the legacy Jasmine package, it seemed to be more robust to upgrade to the `jest.fn` API which behaves exactly the same as `jasmine.createSpy`. The main difference is the `callThrough()` method in Jasmine spies. This behaviour is the default for `jest.fn()`, so the upgrade works as-is.
Linter issues seem to be mostly existing code that is not modified in the diff here (it is likely caused by upgrade to ESLint and TypeScript version 4 -> 5). For now, I will avoid pushing any commits to this branch with a bunch of code formatting changes as it will make it very difficult to read and understand this PR as a whole. |
Thanks for this PR. I will review it in the week-end. Can you maybe reorder the gitlab pipeline steps so that even if lint does not pass, we can see the build and tests succeed ? |
This prevents the linter from blowing up with hundreds of notices about trailing commas in multi-line function arg lists that don’t match the style of the codebase.
That’s a great idea for minimising cost of review/changes here, thanks. I’ve changed order of workflow steps so the linter runs last and added a It’s now much more clear to read the output and see what specific style linting issues are a result of the upgrade. |
Description
Upgrade TypeScript version on the project (hopefully) without requiring major changes to Rollup and Jest configuration.
The main changes are documented in individual commits (happy to squash these if preferable). Getting it to build correctly required a few minor changes to the source to take into account recent changes to enum behaviour, and dealing with several small issues relating to scope ambiguity.
Some minor warnings with peer dependencies and a bunch of linter stuff in the tests could probably be looked at further.
Need to manually test under the following scenarios:
Ink.Story
andInk.Compiler
Ink.Story
andInk.Compiler
Checklist
npm test
).npm run-script lint
).