Change .d.ts files to .ts files

[seancolsen]

Context

• We have an undocumented front end coding standard of placing TypeScript types in *.d.ts in certain specific scenarios.
• Shortly after I joined the team I raised a concern with this practice when talking to @pavish. At the time, my concern was only mild. I mentioned that the practice seemed unconventional, but I didn't have any specific problems with it. Pavish wanted to continue with it, so I let it go.
• Recently, this practice has been bugging me more, and it was one source of slowness during my refactoring in Refactor front end Meta class and a lot of surrounding code. #1109. So I'm raising the concern again, this time with stronger arguments.

Request

• I'd like to stop using *.d.ts files, moving all their contents to *.ts files instead.

• For situations where we want to export types which are relevant to a specific component, we currently have:

  • Icon.svelte
  • Icon.d.ts

  I'd like to change the pattern to:

  • Icon.svelte
  • iconUtils.ts

• For situations where we're re-exporting stuff, we currently have:

  • index.ts
  • types.d.ts

  I'd like to move everything to index.ts.

Rationale

More type safety and easier refactoring

It seems to me that TypeScript does not check *.d.ts files with the same rigor that it checks *.ts files.

Follow me on an example...

1. In src/stores/table-data/types.d.ts, change:

   export type { Display } from './display';

   to

   export type { Display } from './columns';

   Note that columns.ts has no exported Display member. TypeScript sees no problem in with this change in types.d.ts though! Not good. Code like that in a *.ts file would immediately be complaining at me.

2. Running svelte-check produces no errors! Really? That can't be. Let's look at RowCell.svelte which imports Display from '@mathesar/stores/table-data/types'.

   All properties of Display (e.g. handleKeyEventsOnActiveCell) are now typed as any. Ugh! And we don't even get linting errors about the any types!

   This leads to some bugs during refactoring that can be trickier to track down.

In the above example, Display is already exported from src/stores/table-data/index.ts. So within RowCell.svelte, we can import it with:

import type { Display } from '@mathesar/stores/table-data';

I don't see the use of also exporting it from src/stores/table-data/types.d.ts.

Simplicity

It's simpler to have one TypeScript file type. Fewer decisions to make when adding new files. It's especially simpler when re-exporting stuff because all the re-exports can go in one file instead of two.

Easier auto-imports

Let's say I want to use AbstractTypeResponse (defined in App.d.ts). When I let VS Code auto-import that type, it gives:

import type { AbstractTypeResponse } from '@mathesar/App';

But this is problematic because some module loaders will see '@mathesar/App' and have difficulty resolving the ambiguity between '@mathesar/App.svelte' and '@mathesar/App.d.ts'. I've run into this within our codebase before. I can't remember the exact steps I took to reproduce the problem, but I do remember that I had to add .d on the end of my import it to get it to work. It may have been a problem loading imports while running Jest or Storybook. That's additional friction that would not be present when importing from appUtils.ts.

Issues with enums

Some *.d.ts files contain enums. That's problematic because enums are not strictly types -- but also objects used at runtime. In #1109 I hit this issue when running tests from a test.ts files. Jest was unable to find the runtime value for the enum. I had to move TabularType out of a *d.ts file just to get my test to run. All enums should be moved to .ts files. The fact that we even need to think about this demonstrates the additional overhead that comes with our pattern of using *.d.ts files.

Consistency with TypeScript community norms

For example...

• https://www.reddit.com/r/typescript/comments/t23p7o/confused_on_use_of_dts_file_in_reacttypescript_app/

  • If the types are for code you control that's in your application's source directories, they should be defined in regular TypeScript files and consumed via the module system.

  • Declaration files... are for telling TypeScript about code that will exist at runtime that it cannot discover through your own code.

• https://www.reddit.com/r/typescript/comments/r69jmi/whats_the_difference_between_ts_and_dts_file/

  • In your case, you probably just want to write everything in .ts files

  • *.d.ts is typically used to add types to untyped JavaScript

• https://www.reddit.com/r/typescript/comments/rirjiw/when_do_you_use_dts_files_and_when_do_you_use_ts/

  • The .d.ts is a file that let's typescript know the types for a javascript file so you don't have to annotate someone else's code to use the library.

• https://www.reddit.com/r/typescript/comments/rz62fa/dts_files_vs_ts_can_someone_clear_this_up_for_me/?sort=top

  • ...When would I actually write a .d.ts file?...

    ...when you are typing something that's written in Javascript

I couldn't find a single case of someone arguing to use *.d.ts file like we're using them.

[pavish]

The arguments make sense to me regarding not having to use .d.ts and stick with .ts.

However, I do believe we need to try and separate type definition exports from code files wherever possible, for the time-being. Here are my reasons:

We've always had to import types with:

import type { Type } from 'file'

instead of:

import { Type } from 'file'

because our build system does not support it as default, even though it is the typescript convention. I hoped when we can make this change in the future, using separate .d.ts files could help us easily identify the files we need to look out for.

Easier auto-imports
import type { Display } from '@mathesar/stores/table-data';
I don't see the use of also exporting it from src/stores/table-data/types.d.ts.

If we export all type definitions from index.ts, we'd have to write code like this:

1: import { SomeExport } from 'index';
2: import type { SomeType } from 'index';

If we ever type SomeType directly without importing line 2, IDEs will add them to the first line. Not using import type causes vite dev server to behave fuzzily because sometimes it throws an error and sometimes it allows it. Storybook and Jest also behave weirdly. It often goes unnoticed until you see that builds fail. This also affects development time greatly and auto-import has negative benefits in this scenario.

The suggestion of using a single file does not solve the problem at hand which is the slow down of development velocity. The actual solution we need is to be able to do this:

import { someFn, SomeType } from 'file.ts';

without running into issues with vite, svelte, jest and storybook.

When we started the project there was an open issue on the svelte repo regarding this and I believe I read that the error was due to vite. I remember there was an update later which allowed a config value we can set inorder for us to import types directly without using import type.

Whoever works on this issue should find the status of that config, modify our configuration, and then make the other suggested changes mentioned in this issue's description.

@seancolsen

Shortly after I joined the team I raised a concern with this practice when talking to @pavish.

I remember mentioning these points in that call but I don't think you saw the issues around import type vs import back then. This whole thing was a development time saving workaround for that.

I'll leave it to you to go through this comment and ask questions and/or update the issue description.

[seancolsen]

I'm sorry @pavish I still don't understand 😕

Would you be able to provide a concrete example of the problem which I can reproduce (like I did in the "More type safety and easier refactoring" section of my comment)?

[pavish]

@seancolsen Okay, let me try explaining this with an example.

1. Open ConfirmationController.ts.
2. Comment out line 3: import type { Writable } from 'svelte/store';. Note that we have line 4 which imports writable.
3. Now, in line 53: confirmationProps: Writable<ConfirmationProps>;, edit Writable and let VS code auto-import it.
4. You can notice now that line 4 has changed from import { writable } from 'svelte/store' to import { writable, Writable } from 'svelte/store';
5. Notice that our app does not get rendered. A message can be seen on the console: Uncaught SyntaxError: import not found: Writable. This message does not appear all the time, sometimes it just works until it is caught at the later period of time, especially in the previous versions of Svelte which we used when we began working on Mathesar.

The reason behind this is that Svelte (or Vite) does not allow importing type definitions without the use of import type. This type of strict check is not the convention in the TS world, and while auto-importing it leads to IDEs placing them with imports that already exist from the same file.

Inorder to avoid such issues, I began moving type definitions from code to separate files. So, every module would contain an index.ts (or code/logic files) and a type.ts file. This also made code cleaner.

Another decision I took was to use .d.ts like type.d.ts. I planned that when we moved the component-library to it's own package, we could just export these definitions as they are. Since, the files will be compiled to JS while packaging, this seemed the simplest way to go. I did not realize that ts did not treat it with the same rigor that it treats normal .ts files. I'm okay with not using .d.ts. We could still achieve type support by exporting our source with the package.

What I'm in agreement with:

• Rename all .d.ts to .ts. We can do this right away.

What I'm not in agreement with (for the time being):

• Use a single index.ts export and not have any type.ts export.
  • I would like us to be able to do this eventually, but that would mean fixing the issue mentioned earlier first.

I've not kept in touch with the latest updates to this import issue, anyone working on our ticket should figure that out before moving type exports to index.ts.

[seancolsen]

Ok I see what you're talking about now. Thank you for providing the example. Yes I already encounter that problem daily-ish. It's a nuisance, and I can see how it would get worse, were we to put all the re-exports in the same file. I support splitting the re-exports up into separate files -- a file index.ts containing all the re-exported values (including enums), and a file types.ts containing all the re-exported types (only type and interface).

[seancolsen]

I've marked this as "ready" now that we seem to be in agreement about the next steps.

find src -name \*.d.ts

src/stores/table-data/types.d.ts
src/stores/abstract-types/types.d.ts
src/stores/tabs/types.d.ts
src/App.d.ts
src/component-library/cancel-or-proceed-button-pair/CancelOrProceedButtonPair.d.ts
src/component-library/common/types/ComponentAndProps.d.ts
src/component-library/common/actions/types.d.ts
src/component-library/select/Select.d.ts
src/component-library/dynamic-input/types.d.ts
src/component-library/form-builder/types.d.ts
src/component-library/number-input/types.d.ts
src/component-library/tree/Tree.d.ts
src/component-library/file-upload/FileUpload.d.ts
src/component-library/labeled-input/LabeledInput.d.ts
src/component-library/icon/Icon.d.ts
src/component-library/tabs/TabContainer.d.ts
src/component-library/modal/modal.d.ts
src/component-library/types.d.ts
src/global.d.ts

I'm not sure we can change all the .d.ts files to .ts files straight away though. For cases like Icon.d.ts, I think it will present some problems to have Icon.svelte and Icon.ts (as I mentioned in the "Easier auto-imports" section of the ticket body). So I'd like to change those to IconUtils.ts (which will require changes to the dependent files). But this should still be pretty easy.