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

TypeScript Rollout Tier 1 #319

Merged
merged 39 commits into from
Dec 30, 2024
Merged

TypeScript Rollout Tier 1 #319

merged 39 commits into from
Dec 30, 2024

Conversation

kikuomax
Copy link
Collaborator

@kikuomax kikuomax commented Dec 28, 2024

Related issues:

Proposed Changes

  • Bumps version to 0.2.0, because I think the TypeScript migration is drastic enough. It also includes a few breaking changes.
  • Configures TypeScript
    • package.json
      • npm run type-check: Performs type-checking
      • npm run build:dts: Builds the type declaration (dist/buefy.d.ts) file
    • tsconfig: JS files are allowed
    • Rollup
      • A new array JS_COMPONENTS lists all the component groups that are not yet rewritten in TypeScript. Every time a component group migrates to TypeScript, it is removed from JS_COMPONENTS. The array contains all the component groups in this PR.
    • Vite
    • ESLint
    • I chose @microsoft/api-extractor for the type declaration (dist/buefy.d.ts) file generation
  • Replaces Jest with Vitest
  • Replaces .github/workflows/unit_testing.yml with .github/workflows/verify.yml which performs linting, type-checking, and unit-testing, and also does linting, and type-checking of the docs. However, lint errors are tolerated until the TypeScript migration is complete.
  • Removes src/types because they are no longer necessary
  • Renames packages/buefy-next/src/index.js to packages/buefy-next/src/index.ts because tsc requires at least one .ts file. Also renames packages/docs/src/main.js to packages/docs/src/main.ts for the same reason.
  • Prior to other .js files, rewrites packages/buefy-next/src/utils/helpers.js in TypeScript. I chose this file because:
    • There is no dependency on other files in the Buefy project.
    • It has unit tests.
  • Introduces a new file packages/buefy-next/src/utils/vue-augmentation.ts which declares the $buefy Vue global property. A new build script packages/buefy-next/build/augment-dts.mjs transforms the vue-augmentation.ts file and appends it to the type declaration (dist/buefy.d.ts) file.

- Installs `rollup-plugin-esbuild` that adds TypeScript support to
  Rollup. This package supersedes `@rollup/plugin-typescript`.

  Replaces `rollup-plugin-vue` with `@vitejs/plugin-vue` because it is
  no longer maintained. `@vitejs/plugin-vue` supersedes it.

  There were some possible combinations of plugins for TypeScript and
  Vue, though, my preliminary experiments showed that the combination of
  `rollup-plugin-esbuild` and `@vitejs/plugin-vue` looks promising.

  Installs a couple of additional packages:
    - `vue-tsc`: to facilitate type check
    - `@vue/tsconfig`: to bootstrap TypeScript config

- Introduces a new script `type-check` that runs `vue-tsc` to perform
  type check without emitting results.
- Removes `src/types` folder because we will rewrite all the components
  in TypeScript and type declarations will be generated automatically.
Configures `rollup.config.mjs` so that Rollup can process TypeScript in
both .ts and .vue files. Replaces `babel` with `esbuild` that has
built-in support for TypeScript.

`tsconfig.json` is a basic configuration for TypeScript. It has
`allowJs` option `true` since we have bunch of JavaScript files,
though, this option shall be removed when we finish migration to
TypeScript.

No type declaration files (.d.ts) are generated for now. We have to do
it in another script.

Renames `src/index.js` to `src/index.ts`, because TypeScript requires
at least one `ts` file included in the source files.
- `src/index.ts` imports `App` type from `vue`. It reduces one error by
  the `type-check` script.
- Installs and configures `vitest`. Only `*.spec.ts` files are
  evaluated.
`type-check` script uses a new TypeScript config `tsconfig.test.json`
that includes `*.spec.*` files.

`tsconfig.json` adds `*.spec.*` to `exclude` option to exclude
`*.spec.ts` files.
Adds a new script `build:dts` that generates and rolls up type
declarations. The rolled up type declaration is emitted to a new place
`dist/buefy.d.ts`. `build` script also runs `build:dts`.

Uses `@microsoft/api-extractor` (`api-extractor`) to roll up type
declarations. `api-extractor.json` is the configuration file for
`api-extractor`.

Introduces a new TypeScript config `tsconfig.types.json` that enables
options for generating type declarations while disabling transpilation.
- Configures ESLint to run on TypeScript (*.ts) files.

  Installs the following packages necessary for TypeScript processing:
    - @typescript-eslint/eslint-plugin
    - @typescript-eslint/parser

  Updates `.eslintrc.js` according to the following instructions:
    - https://typescript-eslint.io/getting-started#step-2-configuration
    - https://typescript-eslint.io/linting/troubleshooting/#i-am-running-into-errors-when-parsing-typescript-in-my-vue-files

  Some ESLint errors are detected due to introduction of the TypeScript
  rules.
- Changes the entry file of `@ntohq/buefy-next` in the development mode:
  `index.js` → `index.ts`

  Adds `.ts` to `resolve.extensions`.
- Adds a new script `type-check` that checks types. `tsconfig.json` is a
  basic configuration for TypeScript. `allowJs` option in
  `tsconfig.json` shall be removed after TypeScript migration completes.
Fixes and works around type errors in `main.ts`:
- Removes `vueApp.config.productionTip` because it no longer exists
  in Vue 3. See https://v3-migration.vuejs.org/breaking-changes/global-api.html#config-productiontip-removed
- Works around type errors related to tiny-emitter
- Augments Vue component instance so that `$http` and `$eventHub` are
  available in every component.
- `build` script runs both `type-check` and `vite build` in parallel.
  Introduces a new script `build-only` that runs only `vite build`.

  Installs `npm-run-all2` to parallelly runs scripts.
- To prevent version discrepancies, hoists the following dependencies
  used in both `buefy-next` and `docs` packages:
    - `vue`
    - `vite`
    - `@vitejs/plugin-vue`
    - `vue-tsc`
    - `@vue/tsconfig`
    - `sass`
Installs missing type definitions that `packages/docs` depends on:
- `@types/lodash`
- `@types/scrollreveal`
- `@types/cleave.js`: Although the installed version of `cleave.js` is
  1.0.1, `@types/cleave.js` v1.4.12 is installed. Because older versions
  did not correctly declare its types. It should not harm as long as the
  actual usage of `cleave.js` does not change.
- `@types/sortablejs`: I have no idea about which version of
  `@types/sortablejs` I should install, though, it should not matter as
  long as the code actually using `sortablejs` does not change, and
  type-checking passes.
- Applies npm audit suggestions
Introduces TypeScript project references so that `packages/docs` can
import files and declaration files from `packages/buefy-next`. The new
`tsconfig.json` in the root folder defines which tsconfig files are
included in the project. As we focus on type declarations of
`packages/buefy-next`, only `tsconfig.types.json` is included from
`packages/buefy-next` into the project. `packages/docs` introduces a new
`tsconfig.types.json` file that is configured not to import types from
`packages/buefy-next/dist` but to directly include
`package/buefy-next/src` during type check.
`packages/docs/tsconfig.types.json` is also included in the project.
These tsconfig files must have "composite" and "declaration" `true`.

Please refer to TypeScript documentation for more details about
"Project Reference".
https://www.typescriptlang.org/docs/handbook/project-references.html

`.gitignore` ignores `*.tsbuildinfo` files that are generated if project
references are enabled.

In `packages/docs`, `type-check` now uses `tsconfig.types.json` as the
TypeScript configuration to directly include `packages/buefy-next/src`.
`build` script calls a new script `build:type-check` that uses
`tsconfig.json` as the TypeScript configuration and imports types from
`packages/buefy-next/dist` instead.

Also fixes the error in `packages/buefy-next/tsconfig.types.json` that
"emitDeclarationOnly" was turned on while "noEmit" is `true`. Sets
"noEmit" `false`.
- Lets `api-extractor` fail if there is any compiler error, but not fail
  if there is any TSDoc related warnings even in production; we get a
  lot of such warnings due to missing release quality tags; i.e.,
  `@alpha`, `@beta`, `@public`, and `@internal`.
Includes `vite/client` in `compilerOptions.types` so that Vite specific
qualifiers can be properly typed; e.g., `?raw`. Also includes "jsdom"
to suppress `window` related errors.

`tsconfig.types.json` extends `tsconfig.json` instead of
`@vue/tsconfig/tsconfig.dom.json` to avoid discrepancies between
development and production.

Adds "@/" as an alias of the `src` folder to `paths` in both
`tsconfig.json` and `tsconfig.types.json`.
- Fixes the type error caused at the line where components are installed
  to the Vue app. `components/index.js` is renamed to
  `components/index.ts` and exports an array of components as default.
  `index.ts` extracts components from the default export of
  `components/index.ts` instead of enumerating all the exported items
  from `components/index.ts`.
- Exports individual components from `components/index.ts` and
  `index.ts` so that users can benefit from type checking. For now,
  users have to import individual Buefy components they use to check
  types of the components; type checking won't work on components
  globally installed by `App.use(Buefy)`. `components/index.ts` used to
  export only install functions of components.
- `.gitignore` ignores the `temp-dts` folder that temporarily keeps type
  declarations.
Introduces a new file `utils/vue-augmentation.ts` that declares the
module augmentation for `Vue` so that Vue components can access the
Buefy global API through `this.$buefy`. It augments
`ComponentCustomProperties` to include `$buefy`. `$buefy` only has
`globalNoticeInterval` and other fields typed `any` for now, but must
be refined in the future. This file will result in
`vue-augmentation.d.ts` after TypeScript processing, which must be
appended to `buefy.d.ts` because `api-extractor` won't include the
module augmentation in the rolled up output. A new script
`build/augment-dts.mjs` does this job, and `build:dts` script runs
`build/augment-dts.mjs` after `api-extractor` outputs `buefy.d.ts`.
`build/augment-dts.mjs` excludes lines starting with `import` from
`src/utils/vue-augmentation.d.ts` before appending it to the
declaration file body. Because the imported paths should be already in
the declaration file body and relative imports will spoil those types.

`index.ts` includes `utils/vue-augmentation.ts` to activate the module
augmentation.
Replaces `module` field of `package.json`: `dist/esm/index.js` →
`dist/buefy.esm.js`.
- Fixes the error that `build/vetur.js` failed to load the Tag API from
  `.js` file because the Tag API was replaced with the `.ts` file. Tries
  to read `.ts` file first, and then falls back to `.js` file. This
  workaround will become unnecessary when we complete TypeScript
  migration.
Works around the type error in `ExProgrammatically` example of
`Datepicker`. Adds `<script>` section without the language prop to make
it a JavaScript component. This has to be reverted after we complete
TypeScript migration.
- Adds test cases for `matchWithGroups` defined in `utils/helpers.js`.
  As rewriting `matchWithGroups` in TypeScript will involve some changes
  in the flow, we should define tests before rewriting it.
- Adds tests for `toCssWidth` defined in `utils/helpers.js`. As
  rewriting `toCssWidth` in TypeScript will involve a behavioral change
  to it, we should define tests for it. The change is how `NaN` is
  handled:
    - before: `NaN` is returned as `NaN` (number)
    - after: `NaN` is returned as "NaN" (string)

  By the above change, we can declare the return value as `string` or
  `null`. Passing `NaN` to `toCssWidth` is erroneous anyway.

  Also updates `utils/helpers.js` to pass the tests.
- This commit is just for the record of the name change.
Rewrites `utils/helper.js` in TypeScript:
- Augments `App` in `@vue/runtime-core` so that `__VUE_I18N_SYMBOL_`
  introduced by `vue-i18n` may be optionally handled.
- Adds `@internal` directives in TSDoc comments. By adding `@internal`
  directives, `api-extractor` somehow failed with the following error:

    ERROR: Internal Error: Unable to analyze the export "getValueByPath" in
    /buefy/packages/buefy-next/temp-dts/utils/helpers.d.ts
    You have encountered a software defect. Please consider reporting the issue to the maintainers of this application.

  If I removed the `@internal` scope from `getValueByPath`,
  `api-extractor` started to fail with a similar error around `mod`. The
  error disappered by removing the `@internal` scope from
  `getValueByPath` and moving `export` statements to the function
  definitions of `mod`, `bound`, and `hasFlag`.
- `toCssWidth` tweaks `isNaN(width)` as `isNaN(+width)` to avoid the
  type error caused because `isNaN` does not accept a string.
- `matchWithGroups` makes sure `pattern` contains group names, otherwise
  throws `RangeError`. It also removes a redundant check of group names
  that should never become `true`.
- Introduces a new utility type `DeepPartial` to properly type
  `mergeFn`.
- `isCustomElement` picks only the `$root` field of
  `ComponentPublicInstance`. We may face type errors if we use
  `ComponentPublicInstance` with default type parameters.
- Introduces a new interface `MultiColumnSortPriority` which represents
  the element type of the `sortingPriority` parameter of
  `multiColumnSort`. And replaces the type of `sortingPriority`:
  `string[]` → `MultiColumnSortPriority[]`.
- Introduces a new interface `TranslateTouchAsDragEventOptions` which
  represents the `options parameter of the `translateTouchAsDragEvent`
- Introduces a new type utility `ExtractComponentProps` that extracts
  the props type from a component constructor.
- Introduces a new helper type `ExtractComponentData` that extracts the
  data type from the constructor of a component. It may help us avoiding
  use of `any` when we `mount` or `shallowMount` a component that has
  mixins in tests using Vue test utils. As far as I tested, `mount` and
  `shallowMount` do not respect data fields in mixins.
- `isVueComponent` narrows the parameter `c` to
  `ComponentPublicInstance`.
- `isDefined` makes sure that the argument is not `undefined` by type
  narrowing
- Rewrites tests for `utils/helpers.ts` in TypeScript:
  `utils/helpers.spec.js` → `.ts`:
    - Imports necessary functions and object from `vitest`
    - Replaces `jest.fn` with `vi.fn`
    - Gives types to the comparison function given to `indexOf`
    - Specifies type parameters to `merge` calls
    - Adds a test case for `merge`, which needs `DeepPartial` utility
      type to pass type check
The `unit` npm script now runs `vitest`. It no longer updates snapshots.
If you need to update snapshots, use a new npm script `unit:update` or
the `test` script.
Increments the minor version, because I think the TypeScript migration
is drastic enough and includes a few breaking changes.
Introduces a new GitHub Actions workflow `verify.yml` which performs
linting, type-checking, and unit testing when a commit is pushed or a
pull request is made to a specific branch. It will replace
`unit_testing.yml` when the TypeScript migration is complete.

Although the `migrate-to-typescript` is included in the trigger
branches, it should be removed when the TypeScript migration is
complete.
The `verify.yml` workflow introduces a new job `verify-docs` which
is triggered when commits are pushed to, or a PR is made to the `dev`
or `main` branch, and runs the `lint` and `type-check` scripts on the
`docs` package.
The `verify.yml` workflow is no longer triggered by the
`migrate-to-typescript` branch.
The `verify.yml` workflow skips linting. Revert this at the end of the
TypeScript migration.
Removes the `unit_testing.yml` workflow, because `verify.yml` does the
jobs instead.
Updates the development guideline in `.github/CONTRIBUTING.md`.
Remarkable changes:
- Gives separate instructions for the library (`packages/buefy-next`)
  and the documentation (`packages/docs`)
- Introduces type-checking command
- Includes testing and linting in the points to be considered before a
  PR submission
- Increases the minimum Node.js version to 16 which is required by some
  of `devDependencies`. See the commit
  c9aad5b.
- Removes leading `$`s from the code snippet so that readers can replay
  the commands on their shell by copy&paste without editing them
Updates `rollup.config.mjs` so that it can bundle files that have
already migrated to TypeScript. `rollup.config.mjs` should be
incrementally updated when it comes to being merged to `dev` branch;
every time you rewrite a component in TypeScript, you have to remove the
component category from `JS_COMPONENTS`.

So that we can merge PRs in arbitrary order to rollout TypeScript
migration of components one by one, I decided to list .js components
rather than .ts components. Because I thought removing lines for
`JS_COPONENTS` are less ambiguous compared to adding lines to
`TS_COMPONENTS` in terms of ordering.
@kikuomax kikuomax changed the title Ts rollout tier 1 TypeScript Rollout Tier 1 Dec 29, 2024
@kikuomax kikuomax requested a review from wesdevpro December 29, 2024 07:27
@kikuomax kikuomax marked this pull request as ready for review December 29, 2024 07:27
@wesdevpro
Copy link

LGTM 🚀

@kikuomax kikuomax merged commit 7c9a716 into ntohq:dev Dec 30, 2024
18 checks passed
@kikuomax kikuomax deleted the ts-rollout-tier-1 branch December 30, 2024 08:26
@kikuomax
Copy link
Collaborator Author

We got the following error during the publish_dev_to_gpr.yml workflow.

Error: Cannot find module @rollup/rollup-linux-x64-gnu. npm has a bug related to optional dependencies (npm/cli#4828). Please try npm i again after removing both package-lock.json and node_modules directory.

This discussion comment might help.

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.

TypeScript Rollout Tier 1 - ground work (build scripts, TypeScript configuration, etc.)
2 participants