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 to v5 #1053

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

maetl
Copy link
Contributor

@maetl maetl commented Apr 6, 2024

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:

  • Checkout, install and run tests from fresh clone of the repo
  • Compile from TS source and build a releasable version of the package
  • Install as npm package into separate TS project and successfully import Ink.Story and Ink.Compiler
  • Install as npm package into separate JS project and successfully import Ink.Story and Ink.Compiler
  • Copypasta compiled JS dist files into non NPM project and confirm they work when imported via script tag

Checklist

  • The new code additions passed the tests (npm test).
  • The linter ran and found no issues (npm run-script lint).

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.
@maetl
Copy link
Contributor Author

maetl commented Apr 6, 2024

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.

@smwhr
Copy link
Collaborator

smwhr commented Apr 6, 2024

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.
@maetl
Copy link
Contributor Author

maetl commented Apr 6, 2024

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 .prettierrc.json with trailingComma: "es5" to remove hundreds of trivial warnings that were making the linter output hard to follow.

It’s now much more clear to read the output and see what specific style linting issues are a result of the upgrade.

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

2 participants